Move clientconfig into clientv3 so that it can be reused by both etcd…

[ahrtr]

Part of 13624, and it's one of the follow-ups to PR 13635.

Move the original clientConfig from etcdctl to clientv3, so that it can be reused by both etcdctl and v3 discovery.

Note that etcd uses golang's standard package flag to process command line flags, while etcdctl uses github.com/spf13/pflag, and etcdctl and v3 discovery has different flag names, such as --endpoints vs --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 @serathius @ptabor @spzala

[codecov-commenter]

Codecov Report

Merging #13751 (1a3822f) into main (5ed7f00) will decrease coverage by 0.06%.
The diff coverage is 69.82%.

@@            Coverage Diff             @@
##             main   #13751      +/-   ##
==========================================
- Coverage   72.78%   72.71%   -0.07%     
==========================================
  Files         467      467              
  Lines       38279    38283       +4     
==========================================
- Hits        27862    27839      -23     
- Misses       8622     8637      +15     
- Partials     1795     1807      +12     

Flag	Coverage Δ
all	72.71% <69.82%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
etcdctl/ctlv3/command/check.go	12.61% <0.00%> (ø)
etcdctl/ctlv3/command/make_mirror_command.go	14.15% <0.00%> (ø)
etcdctl/ctlv3/command/util.go	17.97% <0.00%> (ø)
etcdctl/ctlv3/command/global.go	58.66% <85.36%> (ø)
etcdctl/ctlv3/command/move_leader_command.go	71.05% <100.00%> (ø)
server/embed/config.go	73.57% <100.00%> (+0.27%)	⬆️
server/embed/etcd.go	74.95% <100.00%> (ø)
server/etcdmain/config.go	85.26% <100.00%> (ø)
server/etcdserver/api/v3discovery/discovery.go	80.27% <100.00%> (ø)
client/v3/txn.go	73.33% <0.00%> (-26.67%)	⬇️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5ed7f00...1a3822f. Read the comment docs.

[ahrtr]

All test are green now. Please take a look. @serathius @ptabor @spzala

[serathius]

Overall looks much better than previous PR, with clearer PR we can see one additional problem clarity of public client interface. client/v3 package already includes Config struct for configuration of client library, having additional ClientConfig inside makes things confusing.

As a really good refactor I would normally merge it as a gradual improvement, however with this adding to public package interface I'm not sure if we can leave things as they are without long term plan how client configuration should look at in the end. This is because I expect that this package cannot introduce a breaking change in configuration. client.Config and client.ClientConfig are similar enough that are sure to confuse users.

I see two things:

• Propose a new version of client.Config that can be used for discovery.
• Postpone the merge and find a less user facing package for ClientConfig

cc @spzala @ptabor

[ahrtr]

Yes, it's a valid concern. We have two solutions for now,

1. We do not need this PR, keep everything it as it's, which means that etcdctl and v3 discovery will have similar/duplicate clientConfig.
2. Add comment to explain the difference between Config and ClientConfig. Currently ClientConfig is only used by etcdctl and v3 discovery. If users want to develop their own applications which need to process command-line flags, then ClientConfig may be useful as well. We could also move the ClientConfig to a separate file such as client_config.go. If we follow this approach, we can't rashly refactor the Config (such as merge some common fields between Config and ClientConfig), because it may have already been used by some users' applications.

Please let me know if you have any better proposal.

[ptabor]

The crucial different between:

"Config" and "ClientConfig" is that "Config" is mostly a resolved configuration, i.e. it contains objects likes Loggers, TLS that are not serialisable any longer (and especially directly) and that can be potentially shared between objects.

The ClientConfig is supposed to be fully declarative configuration that is serializable to JSON or set of flags, and after serialization and deserialization the same exact object can be reached.

I'm leaning towards:

1. Naming ClientConfig as e.g. 'ClientSpecor justclientv3.Spec`- such that it's clean that the object is just declarative description.
2. Providing resolution that converts ClientSpec->Config [edited] , i.e. NewConfigFromSpec(spec)

[ahrtr]

Thanks @ptabor.

It makes sense to change ClientConfig to Spec (already done!), it can also mitigate the confusion to users pointed out by @serathius .

But I do not see any need or use case to convert Config -> Spec for now. Instead, we should provide resolution that convert Spec to Config. It's a very natural process. The command-line flags go to Spec firstly, then convert it to Config afterwards. Since it may also impact the existing etcdctl, so I'd like to do it in a separate PR. WDYT? @ptabor @serathius @spzala

[ahrtr]

Does it make more sense to rename ClientConfig to ConfigSpec? Spec is just a generic spec, but ConfigSpec is a spec specific to client config.

I changed the ClientConfig to ConfigSpec. Please kindly let me know if you have any comments.

[serathius]

Great change! Thanks for the followups!

[ahrtr]

cc @spzala @ptabor

[ptabor]

Thank you.

[ahrtr]

All test passed (we can ignore the semaphoreci failure, which is due to dockerhub rate limit). PTAL cc @serathius @ptabor @spzala

[ahrtr]

All test passes after re-triggering the pipeline.