-
Notifications
You must be signed in to change notification settings - Fork 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
fix consul stanza parsing when a configuration directory is used #5817
Conversation
8d684ef
to
d1ee78a
Compare
@@ -14,6 +14,15 @@ import ( | |||
"github.com/hashicorp/nomad/nomad/structs/config" | |||
) | |||
|
|||
// ParseConfigDefault returns the configuration base | |||
func ParseConfigDefault() *Config { | |||
return &Config{ |
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.
Are there more than just these three that need to have defaults set correctly?
Would be good to add a comment clarifying when new fields with defaults should be added to this return struct.
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.
These are the only three where the defaults are required before decoding, other defaults are set in command.readConfig
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.
One clarifying question, otherwise lgtm
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.
lgtm - I'd do some manual testing too.
The use of public functions here is odd, e.g. ParseConfigDefault
doesn't need to be public, but neither does most parsing functions there I think.
if err != nil { | ||
return nil, err | ||
} | ||
return defaults.Merge(config), nil |
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.
nitpick: I would consider moving this logic into LoadConfigDir
.
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
When parsing a configuration directory, consul, vault, and autopilot stanzas are parsed correctly only if they are in the last file loaded. This change restores correct default merging behavior.