refactor stringSlice type CLI arguments#12457
Conversation
tklauser
left a comment
There was a problem hiding this comment.
Please also provide a short description why this change was done (not just what it does) in the commit message and PR description.
There was a problem hiding this comment.
The len of a slice can never be smaller than 0. I think you can just drop this if clause and let the entry.Infof below print the key with an empty value.
There was a problem hiding this comment.
This will print the option in the format --option=[value1 value2 value3] due to the %v verb being used. It would be nice to get a comma-separated list here so it matches the way the option was passed on the command line.
fef08e4 to
6a85ac3
Compare
|
Thanks for reviewing, I've updated the PR. Please take a look.
|
tklauser
left a comment
There was a problem hiding this comment.
Thanks for the updated commit messages. Got one more suggestion inline regarding the actual fix.
There was a problem hiding this comment.
I think we don't need to wrap the option list with [...] in the output. Also, we should still report options with empty values. How about making this a bit more concise, e.g. as follows?
| msgs := viper.GetStringSlice(k) | |
| if len(msgs) > 0 { | |
| entry.Infof(" --%s='[%s]'", k, strings.Join(msgs, ",")) | |
| continue | |
| } | |
| msg := viper.GetString(k) | |
| if len(msg) > 0 { | |
| entry.Infof(" --%s='%s'", k, msg) | |
| } | |
| v := viper.GetStringSlice(k) | |
| if len(v) > 0 { | |
| entry.Infof(" --%s='%s'", k, strings.Join(v, ",")) | |
| } else { | |
| entry.Infof(" --%s='%s'", k, viper.GetString(k)) | |
| } |
There was a problem hiding this comment.
Looks good! I've update the PR, please take a look.
Signed-off-by: JieJhih Jhang <jiejhihjhang@gmail.com>
6a85ac3 to
dcb6589
Compare
|
test-me-please |
aanm
left a comment
There was a problem hiding this comment.
Nice finding. I think we also need to address GetIntSlice and all other Slices in case we start using it in the future.
Signed-off-by: JieJhih Jhang jiejhihjhang@gmail.com
The type of option
--log-driveris string slice, which make this lineentry.Infof(" --%s='%s'", k, viper.GetString(k))will get the empty string. For the options should handle both type string and string slice, otherwise just print the empty string at the final.Fixes: #12132