refactor(etcdutils): encapsulate client settings in ClientConfig struct#431
Conversation
5332a7e to
aef76fd
Compare
| // Option configures a clientv3.Config built by the helpers in this package. | ||
| // It is the single, backward-compatible extension point that callers use to add | ||
| // TLS (WithTLS) without changing positional signatures. | ||
| type Option func(*clientConfig) |
There was a problem hiding this comment.
It seems that it is and will only be used for TLS thing? Note that etcd-operator doesn't care about any other parameters/options provided by users.
ref: #93
There was a problem hiding this comment.
It was kind of over-designed. How about changing signature to
func MemberList(eps []string, *tls.Config) (*clientv3.MemberListResponse, error) {
instead of
func MemberList(eps []string, opts ...Option) (*clientv3.MemberListResponse, error) {
There was a problem hiding this comment.
func MemberList(eps []string, *tls.Config) (*clientv3.MemberListResponse, error) {
This might not be extensible. I just realized that my previous comment #431 (comment) confused etcd itself's options with the client options used by etcd-operator to connect to etcd. This specific change should be the latter, but I think we need to resolve both cases.
There was a problem hiding this comment.
the client options used by etcd-operator to connect to etcd
There was a problem hiding this comment.
ConfigSpec might not be the best choice for an input parameter. Since SecureConfig contains TLS file paths, but the operator doesn't actually need these files (it can generate tls.Config directly), using ConfigSpec seems unnecessary. If the goal is extensibility, I believe we should just use clientv3.Config directly. (https://github.com/etcd-io/etcd/blob/6006f405800929b5e7e839e7a821d608a311579f/client/v3/config.go#L28)
However, using clientv3.Config directly exposes too many options to the caller, even if the caller is the operator itself. Alternatively, we could encapsulate ep []string and *tls.Config into a new struct, such as etcdClientConfig. This would allow us to add more client configuration fields in the future if needed.
There was a problem hiding this comment.
Since SecureConfig contains TLS file paths, but the operator doesn't actually need these files (it can generate tls.Config directly), using ConfigSpec seems unnecessary.
Right. The point is that we need to support all the options in ConfigSpec to be configurable.
we could encapsulate ep []string and *tls.Config into a new struct, such as etcdClientConfig
OK, we can proceed with this for now.
| } | ||
| cancel() | ||
| }() | ||
| defer func() { closeAndCancel(c, cancel) }() |
There was a problem hiding this comment.
If you move such kind of minor & clear change into a separate PR, it will be merged right away.
aef76fd to
c203d0b
Compare
silentred
changed the title
c203d0b to
500e0c0
Compare
Signed-off-by: shenmu.wy <shenmu.wy@antfin.com>
500e0c0 to
de8f26a
Compare
silentred
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahrtr, silentred The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
2 participants
sub-task 2 of #428
link to #409 and #362
/cc @ahrtr @nwnt