Remove worker related flags from GetControllerFlags() #4279
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
twz123
force-pushed
the
clean-controller-flags
branch
from
57142cd
to
a158006
Compare
jnummelin
previously approved these changes
Apr 19, 2024
kke
reviewed
Apr 19, 2024
twz123
force-pushed
the
clean-controller-flags
branch
4 times, most recently
from
58d624d
to
6e6aef5
Compare
This makes it obvious if something on the command line interface changes (modulo changes to hidden flags and such). Signed-off-by: Tom Wieczorek <[email protected]>
The data-dir is fetched manually from the flag sets nowadays, so the global variable was not used at all. Also use the standard pflag defaulting mechanism for printing the defaults. Signed-off-by: Tom Wieczorek <[email protected]>
Those are also part of GetWorkerFlags(). In fact, the two commands that
use GetControllerFlags() ("controller" and "install"), both also add
GetWorkerFlags(), defining them effectively twice.
Signed-off-by: Tom Wieczorek <[email protected]>
twz123
force-pushed
the
clean-controller-flags
branch
from
6e6aef5
to
756001d
Compare
twz123
force-pushed
the
clean-controller-flags
branch
from
756001d
to
89723c9
Compare
kke
previously approved these changes
May 6, 2024
This will reject logging settings for unknown component names and introduces a deterministic output in the Cobra command's help texts. Remove the unused kube-proxy component from logging. Signed-off-by: Tom Wieczorek <[email protected]>
twz123
force-pushed
the
clean-controller-flags
branch
from
89723c9
to
01c7bc9
Compare
kke
approved these changes
May 29, 2024
There was a problem hiding this comment.
Change looks good and it is a good idea to have tests to detect changes in the user facing API aka the CLI flags.
But I think doing that outside of go (or having large chunks of xample output embedded in go tests) via some "golden tests" / characterization testing might be something to explore.
In fact, having something like that in place before this PR would make this PR very easy to validate.
|
Hmm, for a start, we can as well just have the output in separate files and use go:embed or os.ReadFile in the tests to load those. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Those are also part of
GetWorkerFlags(). In fact, the two commands that useGetControllerFlags()("controller" and "install"), both also addGetWorkerFlags(), defining them effectively twice. Add tests for the--helpoutput to prove that nothing changed. Make logging CLI option statically typed, which will reject logging settings for unknown component names and introduces a deterministic output in the Cobra command's help texts. Remove the unused kube-proxy component from logging.Type of change
How Has This Been Tested?
Checklist: