-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Kpt location parsing #2688
Merged
Merged
Kpt location parsing #2688
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
loudej
requested review from
droot,
mengqiy,
mortent and
phanimarupaka
as code owners
January 31, 2022 05:13
loudej
force-pushed
the
kpt-location-reference
branch
3 times, most recently
from
January 31, 2022 09:25
33f31f0
to
23244f5
Compare
martinmaly
reviewed
Jan 31, 2022
loudej
force-pushed
the
kpt-location-reference
branch
from
February 1, 2022 07:11
9bde556
to
1816383
Compare
- location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...}
- using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed.
- location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock
- for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together
- Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine
loudej
force-pushed
the
kpt-location-reference
branch
from
February 1, 2022 07:19
1816383
to
6891e1c
Compare
martinmaly
approved these changes
Feb 2, 2022
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can start here and iterate. I do worry about the canonical or roundtripping representations and would prefer if the interfaces were a bit more opaque, but we can learn how this work in practice and iterate from here.
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
added a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
added a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
added a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
martinmaly
added a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
pushed a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
* Building block for parsing arg location - location.ParseReference to be used by CLI where string arg is taken - may be used by callers that needs similar arg parsing/muxing - callers which have strongly-typed data initialize types like location.Git{...} and location.Oci{...} * Adding test coverage for parsing "-" arg - using WithStdin and WithStdout option must be added by the caller - if they are both present on all calls to ParseReference, then the location returned for "-" would be ambiguous. it depends on which switch or argument is being parsed. * Fixing PR errors related to best practices * Another update for PR coding standard * Adding details needed by kpg pkg get - location.Reference gets Type() and Validate() - kptfileutils has method to make kptfilev1 types from ref and reflock * Selection of parsers is now controlled by options - for kpt CLI it enables parse to be used for many different args - for external code, enables custom and standard parsing together * Adding comments and returning bool from trivial func * Adding headers * Using lock instead of string for argument name * Using filepath.Clean before creating location.Dir{} * Changing parsers to clarify semantics - Parser and Option split into two concepts - Single WithParsers option now takes ordered parsers - StdioParser adjusted so WithStdin and WithStdout options are not sensitive to parser order - the parser result struct documented to clarify what to return and how they combine Co-authored-by: Louis DeJardin <dejardin@google.com>
martinmaly
added a commit
to martinmaly/kpt
that referenced
this pull request
Feb 18, 2022
Preparing for merge of porch branch back into main,
This was referenced Feb 18, 2022
martinmaly
added a commit
that referenced
this pull request
Feb 18, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
location.ParseReference
returninglocation.Reference
for CLI would replace internal git and oci argument parsing in a way that's callable by porch or other clients.In the case that the caller already has structured data, where the location type and individual properties are known, the
location.Git{...}
andlocation.Oci{...}
should be created directly, rather than creating the string notation to parsing.The common interface under createable struct types, with a parse to go from a string and options to the common interface, is based on the pattern from gcrane pkg's
name.ParseReference(name, opts...)
.The ability to mutate/lock references abstractly is intended to be a step towards eliminating methods from the internal remote.Updater and remote.Origin utility interface. The pattern there is taken from the
io.fs
approach where package functions likemutate.Lock(Reference, string) => (ReferenceLock, error)
make it easy to call optional methods that are not supported on every kind of Reference.