(feat) Add validation for Unsupported Loggings Flags for component-base #92394
Conversation
|
Hi @rahulchheda. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
cc: @serathius |
|
/cc @logicalhan |
| formats := fmt.Sprintf(`"%s"`, strings.Join(logRegistry.List(), `", "`)) | ||
| fs.StringVar(&o.LogFormat, logFormatFlagName, defaultLogFormat, fmt.Sprintf("Sets the log format. Permitted formats: %s.\nNon-default formats don't honor these flags: %s.\nNon-default choices are currently alpha and subject to change without warning.", formats, unsupportedFlags)) | ||
| func (o *Options) AddFlags(fs *pflag.FlagSet) error { | ||
| if unsupportedFlagsSet, unsupportedFlags := unsupportedLoggingFlags(); unsupportedFlagsSet != false { |
There was a problem hiding this comment.
This should be checked in Validate and only if logging-format is non default
|
Approach to validate if unsupported flag was set looks ok, did you manage to test it? |
| } | ||
| //unsupportedFlags := fmt.Sprintf("--%s", strings.Join(unsupportedLoggingFlags(), ", --")) | ||
| //formats := fmt.Sprintf(`"%s"`, strings.Join(logRegistry.List(), `", "`)) | ||
| //fs.StringVar(&o.LogFormat, logFormatFlagName, defaultLogFormat, fmt.Sprintf("Sets the log format. Permitted formats: %s.\nNon-default formats don't honor these flags: %s.\nNon-default choices are currently alpha and subject to change without warning.", formats, unsupportedFlags)) |
There was a problem hiding this comment.
We will still need to list all possible unsupported options using VisitAll
There was a problem hiding this comment.
I think we will need 2 functions one returning all unsupported flags and second one returning unsupported flags that were set.
You could also 1 function with argument set, that if true would return only set flags instead of all.
| // unsupportedLoggingFlags validates all the flags provided | ||
| // if any unsupported falg is provided | ||
| // returns true, with list of all unsupported flags | ||
| func unsupportedLoggingFlags() (bool, []string) { |
There was a problem hiding this comment.
We should not return bool here, maybe just check if slice is empty on caller side?
| func (o *Options) Validate() []error { | ||
| // and gets the list of all unsupported flags | ||
| // with all the unsupported flags that are set | ||
| func (o *Options) Validate(fs *pflag.FlagSet) []string { |
There was a problem hiding this comment.
Please don't change prototype of Validate() []error
| unsupportedFlags := fmt.Sprintf("--%s", strings.Join(unsupportedLoggingFlags(), ", --")) | ||
| formats := fmt.Sprintf(`"%s"`, strings.Join(logRegistry.List(), `", "`)) | ||
| fs.StringVar(&o.LogFormat, logFormatFlagName, defaultLogFormat, fmt.Sprintf("Sets the log format. Permitted formats: %s.\nNon-default formats don't honor these flags: %s.\nNon-default choices are currently alpha and subject to change without warning.", formats, unsupportedFlags)) | ||
| func (o *Options) AddFlags(fs *pflag.FlagSet) error { |
There was a problem hiding this comment.
Please don't change prototype of AddFlags(fs *pflag.FlagSet)
|
Please remove "[WIP]" prefix if ready PTAL @liggitt |
|
/retest |
|
Golint error: |
Signed-off-by: Rahul M Chheda <rchheda@infracloud.io>
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rahulchheda, serathius 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 |
|
/retest |
4 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest |
|
/retest Review the full test history for this PR. Silence the bot with an |
Signed-off-by: Rahul M Chheda rchheda@infracloud.io
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes ##92393
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: