-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
embed: Delay setting initial cluster #7517
Conversation
embed/config.go
Outdated
@@ -246,6 +245,9 @@ func (cfg *configYAML) configFromFile(path string) error { | |||
cfg.ACUrls = []url.URL(u) | |||
} | |||
|
|||
if (cfg.Durl == "" && cfg.DNSCluster == "") && cfg.InitialCluster == "" { |
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.
probably have cfg.InitialCluster == cfg.InitialClusterFromName(DefaultName)
instead of == ""
and keep the InitialCluster
line in NewConfig
? otherwise the default will be blank for non-yaml cases
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.
👍
Have left the default and instead clear it once we have parsed the config.
c18262d
to
0dd8f4f
Compare
NewConfig() should sets initial cluster from name but we should clear it in the event that another discovery option has been specified. Fixes etcd-io#7516
0dd8f4f
to
1a91ed0
Compare
Codecov Report
@@ Coverage Diff @@
## master #7517 +/- ##
=========================================
Coverage ? 70.66%
=========================================
Files ? 320
Lines ? 26205
Branches ? 0
=========================================
Hits ? 18518
Misses ? 6246
Partials ? 1441
Continue to review full report at Codecov.
|
lgtm |
NewConfig() sets an initial cluster (potentially using a default name) but we should clear it in the event another discovery option has been specified. PR etcd-io#7517 attempted to address this however it only worked if the name was left as "default". (Completely) Fixes etcd-io#7516
NewConfig() sets an initial cluster (potentially using a default name) but we should clear it in the event another discovery option has been specified. PR etcd-io#7517 attempted to address this however it only worked if the name was left as "default". (Completely) Fixes etcd-io#7516
NewConfig() sets an initial cluster (potentially using a default name) but we should clear it in the event another discovery option has been specified. PR etcd-io#7517 attempted to address this however it only worked if the name was left as "default". (Completely) Fixes etcd-io#7516
embed.NewConfig()
sets the initial cluster from name but we should clear it in the event that another discovery option has been specified.Fixes #7516