-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Adds a new -disable-host-node-id option to help when testing with containers. #2904
Conversation
@@ -1371,6 +1376,9 @@ func MergeConfig(a, b *Config) *Config { | |||
if b.NodeID != "" { | |||
result.NodeID = b.NodeID | |||
} | |||
if b.DisableHostNodeID == true { | |||
result.DisableHostNodeID = b.DisableHostNodeID |
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.
Can simplify as: result.DisableHostNodeID = b.DisableHostNodeID
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.
Unfortunately that'll let a merge with the default value of false
overwrite a previous value of true
. In other words, we use the fact that it's true
to indicate that the user actively set it. We could use a *bool
here, but this way works as well since this defaults to false
.
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.
Ah great point! Thank you for that explanation.
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.
Been working on a little framework to do everything with pointers so we can always tell if the user set it and merge cleanly, but haven't got everything ported to that yet - https://github.com/hashicorp/consul/blob/master/command/base/config_util.go.
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.
Oh nice I will check it out. Is the framework done and just needs to be integrated?
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's close - probably needs some support for a few other data types that show up in the config options.
Fixes #2877.