RFC: Reliable CLI parsing #523
Merged
Conversation
This file contains hidden or 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
Member
|
Already out of date. @giuseppe PTAL |
Member
|
LGTM Also is there any user of |
Member
|
I'd love to drop skopeo layers completely, there have been plenty of time to notice it's deprecated. |
Member
|
Can we drop it from the help/man pages first and see if anyone notices? |
mtrmac
force-pushed
the
cli-parsing
branch
6 times, most recently
from
35ed5a3 to
b869a83
Compare
Contributor
Author
|
This is now finished, including comprehensive tests for |
Member
|
I am fine with making |
Member
|
SGTM |
mtrmac
force-pushed
the
cli-parsing
branch
3 times, most recently
from
e1993f1 to
8274191
Compare
Member
|
@mtrmac What do you want to do with this PR? |
Contributor
Author
@rhatdan Get it merged, ideally. The discussion of the Though, I guess, on balance, I’m willing to live with the code ugliness in order not to break Let me rebase this some time… |
Member
|
@runcom Lets get this merged. |
mtrmac
force-pushed
the
cli-parsing
branch
3 times, most recently
from
f656a77 to
ad0d1fe
Compare
Replace commandTimeoutContextFromGlobalOptions with globalOptions.commandTimeoutContext. This requires passing globalOptions to more per-command *Options state. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
contextFromGlobalOptions now uses globalOptions instead of cli.Context.Global* . That required passing globalOptions through a few more functions. Now, "all" that is left are all the non-global options handled in contextFromGlobalOptions. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is similar to the previous *Options structures, but this one will support differing sets of options, in particular for the copy source/destination. The way the return values of imageFlags() are integrated into creation of a cli.Command forces fakeContext() in tests to do very ugly filtering to have a working *imageOptions available without having a copyCmd() cooperate to give it to us. Rather than extend copyCmd(), we do the filtering, because the reliance on copyCmd() will go away after all flags are migrated, and so will the filtering and fakeContext() complexity. Finally, rename contextFromGlobalOptions to not lie about only caring about global options. This only introduces the infrastructure, all flags continue to be handled in the old way. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We don't want to worry about mismatch of the flagPrefix value between imageFlags() and contextFromImageOptions(). For now, record it in imageOptions; eventually we will stop using it in contextFromImageOptions and remove it again. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is one of the ugliest parts; we need an extra parameter to support the irregular screds/dcreds aliases. This was previously unsupported by (skopeo layers). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This was previously unsupported by (skopeo layers). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This was previously unsupported by (skopeo layers) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This was previously only supported in (skopeo copy). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This was previously only supported in (skopeo copy). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is an extension of imageOptions that carries destination-specific flags. This will allow us to handle --dest-* flags without also exposing pointless --src-* flags. (This is, also, where the type-safety somewhat breaks down; after all the work to make the data flow and availability explicit, everything ends up in an types.SystemContext, and it's easy enough to use a destination-specific one for sources. OTOH, this is not making the situation worse in any sense.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This introduces YET ANOTHER *Options structure, only to share this option between copy source and destination. (We do need to do this, because the libraries, rightly, refuse to work with source and destination declaring its own versino of the --authfile flag.) Signed-off-by: Miloslav Trmač <mitr@redhat.com>
All the contextFromImage{,Dest}Options flags are now defined in
imageFlags/imageDestFlags.
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
contextFromImageOptions is finally not using any string-based lookup in cli.Context, so we don't need to record this value any more. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We no longer need the *cli.Context parameter, and at that point it looks much cleaner to make this a method (already individually; it will be even cleaner after a similar imageDestOptions conversion). Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is analogous to the imageOptions.newSystemContext conversion. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
It was not really any clearer when broken out. We already have a pair of trivial src/dest API calls before this, so adding a similar src/dest call for SystemContext follows the pattern. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We no longer need it for handling flags. Also, require the caller to explicitly pass an image name to parseImage instead of, horribly nontransparently, using the first CLI option. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
That in turn makes sure that the cli.String() etc. flag access functions are not used, and all flag handling is done using the *Options structures and the Destination: members of cli.Flag. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Contributor
Author
|
@rhatdan rebased. |
Member
|
LGTM |
This was referenced Mar 27, 2020
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
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.
@runcom , others: RFC
While testing a related change I have noticed that
skopeo layerssetsDockerDaemonInsecureSkipTLSVerifyalways totrue, because the--tls-verifyoption is not declared forskopeo layers; and the wayurfave/cliis used, there was no connection between the option declaration and reading the value, so it’s way too easy to omit one or the other. (In particular,cli.BoolTcan returnfalseif the option is not found, or on any other error, really, not only when the user has explicitly provided afalsevalue.)So, this motivated a comprehensive rework of the CLI data management; instead of copy&pasting strings and hoping to never make a typo (which is just not good enough for security options), create a
struct someCommandOptionsholding well-typed argument values, and set up thecli.Flagstructures to write data in there; command handlers then use the values directly instead of using string lookup via*cli.Context. See the ‘Create an "options" structure for each command’ commit message for a detailed writeup of the structure change.Given the explicit data structures, we can then fully model an
optionalBoolandoptionalStringargument type, and actually completely distinguish between a missing and default-valued argument values (well, as far as we can get without a native Go “optional” type mechanism).(WIP because I need to finish and clean up the tests.)
Finally, this allows explicitly modeling the shared image-related options
--src-*/--dest-*forcopy, or--*for other commands, and expressing the relationships as explicit data structure pointers. That, however, made it much more visible that--authfile(introduced in #453 , and I’m afraid this design is pretty much my fault) is an irregular case: it is shared across the sources/destinations, but it is not a global flag; that forces all users to manage the scope of the flag explicitly, see howsharedOptsneeds to be added everywhere. @umohnani8 @rhatdan @TomSweeneyRedHat Would it be reasonable to make--authfilea global flag, without backward compatibility (i.e.skopeo --authfile=… copyinstead ofskopeo copy --authfile=…)? I guess not, but I thought I’d at least ask.