pkg/flags: warns on shadowed environment variable flags #8385
Conversation
pkg/flags/flag.go
Outdated
|
|
||
| // WithNoConflict configures no-conflict mode. | ||
| // Expects error on conflicting flags. | ||
| func WithNoConflict() OpOption { |
There was a problem hiding this comment.
is it worth making this optional? I feel like it should always error out on shadowing
| @@ -118,13 +127,18 @@ func verifyEnv(prefix string, usedEnvKey, alreadySet map[string]bool) { | |||
| continue | |||
| } | |||
| if alreadySet[kv[0]] { | |||
| plog.Infof("recognized environment variable %s, but unused: shadowed by corresponding flag ", kv[0]) | |||
There was a problem hiding this comment.
should this be staged as a deprecated warning in 3.3 and then finally fully error out in 3.4?
Signed-off-by: Gyu-Ho Lee <[email protected]>
gyuho
force-pushed
the
shadowed-environment-variables
branch
from
34319bc
to
195744a
Compare
gyuho
changed the title
|
Is there any historical reason why shadowing is only a warning instead of fataling? I want to clamp down on etcd misconfigurations that have environment variables + flags defined, but this will also kill etcd for init setups that work by coincidence. /cc @xiang90 |
|
+1 for just fataling on shadowed variables |
|
@heyitsanthony Discussed with @xiang90 to keep it as warning for 3.3. |
|
@gyuho We cannot panic users to keep this backward compatible. Or users might suddenly fail restarting etcd due to shadowing. |
|
OK, shadowing should be considered a misconfiguration? If so, this upgraded warning message is good to merge. I couldn't come up with any compelling reasons for why anyone would want shadowing besides backwards compat. Is 3.4 good for fatal or should it be deferred to later? |
Address #8380.