GCE e2e build/test passed for commit f7c78bd1d0529b5f1ce7e74fbfd0682af96b8261.

• Build Log
• Test Artifacts
• Internal Jenkins Results

GCE e2e build/test passed for commit 34318fa4991ba16dbc3cee65dc7a91582918280b.

• Build Log
• Test Artifacts
• Internal Jenkins Results

GCE e2e build/test passed for commit 1036902576a900d041cb098a73064af1cbafb7ec.

• Build Log
• Test Artifacts
• Internal Jenkins Results

GCE e2e build/test passed for commit b99bf30b9858f753e71751273d7cba1dbc75b6c3.

• Build Log
• Test Artifacts
• Internal Jenkins Results

I say NAK. We use this syntax for everything. See all of the other calls to GetFlag*

And this is AFTER you removed filename....

I say NAK. We use this syntax for everything. See all of the other calls to GetFlag*

Looking up via name as opposed to binding a variable to flag is causing various parts of our system to start depending on having flagsets (and particular flagsets at that) deeper into the stack that they should. The data is important, but the particular flag used to set is not.

This is producing instability downstream as the data dependency chain is unclear due to the magic container properties of certain flagsets. You can see the effect of this in flags like template, output, and validate.

Given an intent to have composable commands, we cannot continue to depend upon locating particular flags by name in particular flagsets propagated through the stack.

@smarterclayton Anything to add? This contributes to rebase pain as we see failures depending on exactly which branches we take through certain codepaths that rely on finding the correct flagset containing flags with particular names.

See also #7311 for the intent to have composable units that decoupled from flags. Binding to variables to makes this transition easier because it forces the data flow to be explicit.

Making 'filename' a special case is not the answer. I'm sticking with my NAK,

Having an options structure seems much more reasonable and I'd be a fan/fine with that. We already do it with config, attach, exec...

I'd say expand that concept not this one off....

moving the binding to a local var is a step in the direction of an options struct... I agree this isn't where we want to end up, but I think this is a contained change in the right direction

@liggitt it's not though, it's a hack. Create an options struct with a single option, then I'd call it a step in the right direction.

@liggitt it's not though, it's a hack. Create an options struct with a single option, then I'd call it a step in the right direction.

Are you demanding options structs for everything before accepting this? Would a "get no worse" approach be ok? Such an approach would require the func AddJsonFilenameFlag(cmd *cobra.Command, value *[]string, usage string) API change, which ripples through every command regardless. The callers could choose to pass an ignored value through, but would really consider that better?

My take:

• I agree that AddJsonfilenameflag() should take the additional arg.
• You should define a createOptions, deleteOptions, exposeOptions, *Options which contains only the filename slice of strings (which you will hand to AddJsonFilenameFlag)
• Instead of passing that filename slice of string to runCreate, runDelete, run* you pass the *Options.

Seems like it will be small addition to this patch...

Labelling this PR as size/L

@eparis refactored to have a stub Options object.

GCE e2e build/test passed for commit f1b81ff.

• Build Log
• Test Artifacts
• Internal Jenkins Results

GCE e2e build/test passed for commit d54639b5fcbee4e4ae74c524ae58be913ee24d18.

• Build Log
• Test Artifacts
• Internal Jenkins Results

thank you @deads2k do you think it is a (minor) improvement?

thank you @deads2k do you think it is a (minor) improvement?

I do. It makes gathering the low hanging fruit more obvious. I may do the obvious flags for a few commands in followups.

@k8s-bot test this [testing build queue, sorry for the noise]

GCE e2e build/test passed for commit f1b81ff.

• Build Log
• Test Artifacts
• Internal Jenkins Results

Automatic merge from SubmitQueue