-
Notifications
You must be signed in to change notification settings - Fork 494
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
feat(etcd): support external standalone etcd #595
Conversation
Codecov Report
@@ Coverage Diff @@
## main #595 +/- ##
==========================================
+ Coverage 80.06% 80.16% +0.10%
==========================================
Files 95 95
Lines 10956 10961 +5
==========================================
+ Hits 8772 8787 +15
+ Misses 1705 1698 -7
+ Partials 479 476 -3
Continue to review full report at Codecov.
|
pkg/option/option.go
Outdated
@@ -245,6 +276,11 @@ func (opt *Options) Parse() (string, error) { | |||
} | |||
|
|||
opt.renameLegacyClusterRoles() | |||
|
|||
if opt.UseStandaloneEtcd { | |||
opt.ClusterRole = "secondary" |
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 could be more visible / explicit that when using "standalone" etcd, the cluster role cannot be "primary". Logging is not yet initialized here but maybe consider adding a comment:
opt.ClusterRole = "secondary" | |
opt.ClusterRole = "secondary" // when using external standalone etcd, the cluster role cannot be "primary" |
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.
updated
pkg/cluster/cluster.go
Outdated
} else { | ||
if c.opt.UseStandaloneEtcd { | ||
err := c.Put(c.Layout().ClusterNameKey(), c.opt.ClusterName) | ||
if err != nil { | ||
return fmt.Errorf("register cluster name %s failed: %v", | ||
c.opt.ClusterName, err) | ||
} | ||
} else { | ||
return fmt.Errorf("key %s not found", c.Layout().ClusterNameKey()) | ||
} |
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.
to reduce nesting.
} else { | |
if c.opt.UseStandaloneEtcd { | |
err := c.Put(c.Layout().ClusterNameKey(), c.opt.ClusterName) | |
if err != nil { | |
return fmt.Errorf("register cluster name %s failed: %v", | |
c.opt.ClusterName, err) | |
} | |
} else { | |
return fmt.Errorf("key %s not found", c.Layout().ClusterNameKey()) | |
} | |
} else if c.opt.UseStandaloneEtcd { | |
err := c.Put(c.Layout().ClusterNameKey(), c.opt.ClusterName) | |
if err != nil { | |
return fmt.Errorf("register cluster name %s failed: %v", c.opt.ClusterName, err) | |
} | |
} else { | |
return fmt.Errorf("key %s not found", c.Layout().ClusterNameKey()) |
pkg/option/option.go
Outdated
opt.flags.StringVar(&opt.ClusterName, "cluster-name", "eg-cluster-default-name", "Human-readable name for the new cluster, ignored while joining an existed cluster.") | ||
opt.flags.StringVar(&opt.ClusterRole, "cluster-role", "primary", "Cluster role for this member (primary, secondary).") | ||
opt.flags.StringVar(&opt.ClusterRequestTimeout, "cluster-request-timeout", "10s", "Timeout to handle request in the cluster.") | ||
opt.flags.StringVar(&opt.ClusterName, "cluster-name", "eg-cluster-default-name", |
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.
per https://github.com/golang/go/wiki/CodeReviewComments#line-length:
don't add line breaks to keep lines short when they are more readable long--for example, if they are repetitive.
please revert the newly added line breaks.
* feat(etcd): init * fix(statuscontroller): remove log * fix(option): add comment * fix(chart): fix typo * style(option): revert break line
close #584