Make v3 discovery and etcdctl reuse the same ClientConfig #13745
Conversation
Codecov Report
@@ Coverage Diff @@
## main #13745 +/- ##
==========================================
- Coverage 72.72% 72.59% -0.13%
==========================================
Files 467 468 +1
Lines 38286 38309 +23
==========================================
- Hits 27842 27811 -31
- Misses 8635 8694 +59
+ Partials 1809 1804 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
ahrtr
force-pushed
the
reuse_client_config
branch
2 times, most recently
from
1a34acb to
cb4fd9c
Compare
client/v3/config.go
Outdated
| return client | ||
| } | ||
|
|
||
| func NewClientCfg(endpoints []string, dialTimeout, keepAliveTime, keepAliveTimeout time.Duration, scfg *SecureCfg, acfg *AuthCfg) (*Config, error) { |
There was a problem hiding this comment.
Would recommend constructor only be responsible for providing default values and not setting or validating. For configuration structs I expect values to be provided via json.Unmarshall or flags.VarString. Both of those functions take reference to struct/field making the constructor you wrote not very useful.
Example on how to define configuration (based on how K8s code does configuration):
type ClientConfig struct {
DialTimeout time.Duration `json:"dial-timeout"`
}
func NewClientConfig() ClientConfig {
return ClientConfig{
DialTimeout: 5*time.Second,
}
}
func (c ClientConfig) Validate() err {
if c.DialTimeout < 0 {
return fmt.Error("Timeout cannot be negative")
}
return nil
}
func (c ClientConfig) AddFlags(fs flags.FlagSet) {
fs.VarString("dial-timeout", &c.DialTimeout, c.DialTimeout, "Dial timeout")
}
func main() {
config := NewClientConfig()
config.AddFlags(flags.CommandLine) // or something like
err := config.Validate()
if err != nil {
fmt.Print(err)
os.Exit(1)
}
}
This code is very draftly, however shows overall direction. As for registering single ClientConfig as different flags we could use aliasing. For example:
func addClientConfigFlags(c ClientConfig, prefix string, fs flags.FlagSet) {
fs.VarString(prefix + "-dial-timeout", &c.DialTimeout, c.DialTimeout, "Dial timeout")
}
type DiscoveryClientConfig ClientConfig
type EtcdctlClientConfig ClientConfig
func (c DialClientConfig) AddFlags(fs flags.FlagSet) {
addClientConfigFlags(c, "discovery", fs)
}
func (c EtcdctlClientConfig) AddFlags(fs flags.FlagSet) {
addClientConfigFlags(c, "client", fs)
}
There was a problem hiding this comment.
I just moved the function from etcdctl into clientv3, and exported the function. Apart from that, I did not change anything else. If we want to change anything else, I'd like to do it in a separate PR. WDYT?
|
This PR is little hard to read due to lot of dependency changes. Could you please create a separate PR/commiut that just fixes the dependencies? It should be fast and easy to merge, allowing us to focus the discussion on the config changes. |
All the changes in go.mod and go.sum are made by Just as I mentioned in the description of this PR, I moved the package |
ahrtr
force-pushed
the
reuse_client_config
branch
3 times, most recently
from
4b38211 to
91e2c44
Compare
move the original clientConfig from etcdctl to clientv3. The original pks/cobrautil is only used by etcdctl or etcdutl, so move it into the clientv3 package.
Can you create a separate PR to move |
Actually I want to do it in a separate PR as well, but I can't. I mentioned the reason in the description of this PR. Pasted it again. The original |
ahrtr
force-pushed
the
reuse_client_config
branch
from
91e2c44 to
35e2852
Compare
|
I don't think that tools for cobra should be put into client package. We should not put dependency on cobra for users that just want to call etcd API. I think we should update the code to stop using cobrautil as dependency. As |
|
Actually it just depends on cobrautl.ExitBadArgs and cobrautl.ExitBadConnection. If you are OK, I can remove the dependency in this PR by change them with the related number directly in this PR, see below, Then I can update all the dependency related changes in a separate PR. WDYT? @serathius |
|
I think that while trying to reuse ClientConfig we are stumbling upon difference in behavior between By reusing |
|
Overall I agree with you. I just submitted another PR 13751. Thanks. |
Part of 13624, and it's one of the follow-ups to PR 13635.
Move the original clientConfig from
etcdctltoclientv3, so that it can be reused by both etcdctl and v3 discovery.The original
clientConfighas some dependencies on pks/cobrautil, such as (https://github.com/etcd-io/etcd/blob/main/etcdctl/ctlv3/command/global.go#L180). However, clientv3 isn't allowed to depend onpkg, see go.mod#L50. Actually the original pks/cobrautil is only used byetcdctloretcdutl, so move it into theclientv3package as well.Note that etcd uses golang's standard package
flagto process command line flags, while etcdctl usesgithub.com/spf13/pflag, and etcdctl and v3 discovery has different flag names, such as--endpointsvs--discovery-endpoints. So it's difficult to share the same code to process flags between them. So the command line flags keep unchanged in this PR.cc @ptabor @serathius @spzala