-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Switch from arguments to an input structure for kubectl command #106159
Conversation
/triage accepted |
1a8e092
to
baab99d
Compare
/assign @dims |
I am happy to confirm that these changes, from the outside consumer point of view, are exactly what's needed by me. works perfectly and simplifies our code too as well as making the api cleaner. |
/hold |
func (f *ConfigFlags) WithWrapConfigFn(wrapConfigFn func(*rest.Config) *rest.Config) *ConfigFlags { | ||
f.WrapConfigFn = wrapConfigFn | ||
return f | ||
} |
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.
Do we want to allow chaining of these functions? This is really what I meant when I said this should be a slice.
func main() {
flags := genericclioptions.NewConfigFlags(false)
flags = flags.WithWrapConfigFn(func(config *rest.Config) *rest.Config {
fmt.Println("wrap 1")
return config
})
flags = flags.WithWrapConfigFn(func(config *rest.Config) *rest.Config {
fmt.Println("wrap 2")
return config
})
flags.ToRESTConfig()
}
// wrap 2
// wrap 1
// Unsure of the order we'd want here.
Or am I missing something here?
func (f *ConfigFlags) WithWrapConfigFn(wrapConfigFn func(*rest.Config) *rest.Config) *ConfigFlags { | |
f.WrapConfigFn = wrapConfigFn | |
return f | |
} | |
func (f *ConfigFlags) WithWrapConfigFn(wrapConfigFn func(*rest.Config) *rest.Config) *ConfigFlags { | |
if f.WrapConfigFn == nil { | |
f.WrapConfigFn = wrapConfigFn | |
} else { | |
old := f.WrapConfigFn | |
f.WrapConfigFn = func(config *rest.Config) *rest.Config { | |
return old(wrapConfigFn(config)) | |
} | |
} | |
return f | |
} |
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.
Since WrapConfigFn
both accepts and returns *rest.Config
you can chain then together on your own, and pass that into WithWrapConfigFn
that's why I don't think we need or want to make this a slice.
fwiw, for me the change that broke the I adapted my code to use |
/hold cancel |
/lgtm |
the /approve |
/assign @lavalamp |
/approve (for the record) |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dims, eddiezane, smarterclayton, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I've went ahead and added cli-maintaners for the missing bits in #106451 |
What type of PR is this?
/kind cleanup
/priority backlog
/sig cli
What this PR does / why we need it:
This PR:
WithWrapConfigFn
function allowing to provider a wrapper for client configThis is alternative to #104926
Special notes for your reviewer:
/assign @eddiezane
/cc @dovholuknf
Does this PR introduce a user-facing change?