-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CE part of https://github.com/hashicorp/vault-enterprise/pull/4269 #26406
Conversation
CI Results: |
Build Results: |
@@ -1682,11 +1682,17 @@ func (c *ServerCommand) Run(args []string) int { | |||
level, err := loghelper.ParseLogLevel(config.LogLevel) | |||
if err != nil { | |||
c.logger.Error("unknown log level found on reload", "level", config.LogLevel) | |||
goto RUNRELOADFUNCS | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arguably this should be fixed in its own PR, thoughts? The issue as I see it is that goto RUNRELOADFUNCS
only makes sense when we're unable to re-read the config, because the reload steps we take after RUNRELOADFUNCS aren't exclusively dependent on the config file itself having changed - they rely on TLS certs changing or license file changes. But we have some cases where folks have copied the goto
pattern thinking it's what should be done whenever any error occurs as part of reloading. This is undesirable: just because the user gave a bogus loglevel or whatnot, that shouldn't impede reloading other things like Census or the canary write interval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems fine to me to fix this in this PR, under that whole "make things nicer when you're in there" philosophy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -1682,11 +1682,17 @@ func (c *ServerCommand) Run(args []string) int { | |||
level, err := loghelper.ParseLogLevel(config.LogLevel) | |||
if err != nil { | |||
c.logger.Error("unknown log level found on reload", "level", config.LogLevel) | |||
goto RUNRELOADFUNCS | |||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems fine to me to fix this in this PR, under that whole "make things nicer when you're in there" philosophy.
No description provided.