-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix double completion of option sets in CLI #1015
base: main
Are you sure you want to change the base?
Conversation
@@ -70,16 +71,19 @@ type OptionWithSessionCompleter interface { | |||
CompleteWithSession(ctx clictx.OCM, session ocm.Session) error | |||
} | |||
|
|||
func CompleteOptionsWithSession(ctx clictx.Context, session ocm.Session) options.OptionsProcessor { | |||
func CompleteOptionsWithSession(ctx clictx.Context, session ocm.Session, all ...bool) options.OptionsProcessor { | |||
other := general.Optional(all...) |
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.
This conveys no information for me. Why are we using this? What does this accomplish? What is other
? Why is this needed here? What is Complete
doing? What is Configure
doing? This code doesn't explain anything and the names don't mean anything.
Please add some documentation or comments or rename some variables.
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.
see PR docu
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.
The PR docu is not in the code. If I look at this code a couple days from now, I won't be trying to look up the PR to find what's going on in there. Please add documentation directly on the function.
What this PR does / why we need it
CLI Option objects typically feature some completion method. It is used to check values set by CLI options and to provide derived fields.
There are different flavors for such completion methods, without any argument, with some context object or with a session object, according to the requirement of the completion function.
The
Run
method of a CLI command is called by a Cobra wrapper. THis wrapper knows about the context object for the command and already calls the session-less compöletion flavors.Some commands have options whose completion method requires a session object. This cannot be called by the command wrapper, because the existence of a session is specific for dedicated command implementations. Therefore, those command call the function
CompleteOptionsWithSession
to create an option processor completing the configured optio set with a session.The current implementation not only tries the session variant but all other variants, also.
Therefore, completion of options using the standard completion behaviour are completed twice.
The fix changed this behaviour to process the standard completion flavors only if explicitly requested. By default, only the session variant is called (because the others are already called by the used command wrapper.
This problem has be detected, because configured upload handlers (per CLI option) were registered twice.
Which issue(s) this PR fixes