Feature gate CLI flag and visibility changes #4368
Conversation
Signed-off-by: Anthony J Mirabella <[email protected]>
…tor into feature_gates_pt2
Signed-off-by: Anthony J Mirabella <[email protected]>
Signed-off-by: Anthony J Mirabella <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #4368 +/- ##
==========================================
+ Coverage 90.70% 90.73% +0.03%
==========================================
Files 178 179 +1
Lines 10357 10399 +42
==========================================
+ Hits 9394 9436 +42
Misses 745 745
Partials 218 218
Continue to review full report at Codecov.
|
…tor into feature_gates_pt2
Signed-off-by: Anthony J Mirabella <[email protected]>
service/featuregate/flags.go
Outdated
| // Flags adds CLI flags for managing feature gates to the provided FlagSet | ||
| func Flags(flags *flag.FlagSet) { | ||
| flags.Var( | ||
| gatesList, | ||
| gatesListCfg, | ||
| "Comma-delimited list of feature gate identifiers. Prefix with '-' to disable the feature. '+' or no prefix will enable the feature.") | ||
| } |
There was a problem hiding this comment.
I would like to see at least couple of examples for the rules. At least in the func comment.
service/featuregate/gates.go
Outdated
| // GlobalRegistry returns the Registry instance used by package-global functions in this package. | ||
| func GlobalRegistry() *Registry { | ||
| return reg | ||
| } |
There was a problem hiding this comment.
Do you need this to be public?
There was a problem hiding this comment.
There was a problem hiding this comment.
Sure, per the comment there "and have only a "GetRegistry" func that gets the global one". The idea was not to have both global funcs and global Registry, but have only one global func "GetGlobalRegistry" and the Registry itself.
There was a problem hiding this comment.
Also that was called a "prototype" and you sneak it in a PR that we need (adding the flag)... not sure this is the right call.
service/featuregate/gates.go
Outdated
| return reg | ||
| } | ||
|
|
||
| type Registry struct { |
There was a problem hiding this comment.
What is the need in this PR that adds the flag to make this public?
There was a problem hiding this comment.
I'd rather deal with the changes here in one PR than in two. Every open PR I have to carry brings cognitive load and repetitive tasks such as dealing with conflict resolution and upstream changes. The two changes here are to the same component and both were either deferred from or requested after the initial PR that added the gate implementation. Managing separate PRs for them does not make sense.
There was a problem hiding this comment.
You still did not answer why is this needed.
There was a problem hiding this comment.
I understand where this comes from, but my comment was to remove the other global funcs.
There was a problem hiding this comment.
@Aneurysm9 for reasons like #4430 we MUST never accept different changes in same PR.
…tor into feature_gates_pt2
This reverts commit 23c957c.
This reverts commit df64ad2. Signed-off-by: Anthony J Mirabella <[email protected]>
Aneurysm9
force-pushed
the
feature_gates_pt2
branch
from
0de0d52 to
e5e49ce
Compare
Signed-off-by: Anthony J Mirabella <[email protected]>
| type FlagValue map[string]bool | ||
|
|
||
| // String returns a string representing the FlagValue | ||
| func (f FlagValue) String() string { |
There was a problem hiding this comment.
Does this need to be public? (I want to understand)
There was a problem hiding this comment.
Yes. It is part of the flag.Value interface.
There was a problem hiding this comment.
Sorry wrong line of comment, was looking at SetSlice (you can answer on Slack or after the merge).
Adds CLI flags for setting feature gates, exposes the feature gate registry type and provides an accessor for the global feature gate registry.