Skip to content
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

etcdctl: support etcd0.4 #4020

Merged
merged 2 commits into from
Dec 21, 2015
Merged

etcdctl: support etcd0.4 #4020

merged 2 commits into from
Dec 21, 2015

Conversation

xiang90
Copy link
Contributor

@xiang90 xiang90 commented Dec 18, 2015

For CoreOS users, they will get a updated version of etcdctl without updating
the etcd server version. And the users cannot really control this behavior.
We do not want to suddenly break them without enough communication.

So we still want the most basic operations like get, set, watch of etcdctl2 work
with etcd 0.4. This patches solve the incompatibility issue.

/cc @mitak this involves a client API additional change.

/cc @philips @crawford Please take a look and give it a try.

@xiang90 xiang90 force-pushed the ctl_04 branch 2 times, most recently from 141c01d to 2f84eb8 Compare December 18, 2015 03:42
@@ -162,6 +162,11 @@ type Client interface {
// this may differ from the initial Endpoints provided in the Config.
Endpoints() []string

// RestEndpoints resets the set of API endpoints used by Client to resolve
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rest->Reset
but how about just SetEndpoints?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel Reset is slightly better as this is a client Interface and we want the func name to be more specific.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dunno, how is it more specific? To me Reset has more of an implication that it's reverting to some default/initial state, whereas x.Set(y) is very explicitly "set the state of x to y"

I don't feel extremely strongly about this but curious about your reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh. OK. I feel Reset means set something again (not set it initially). I trust your english. So I will change it to set then.

@jonboulle
Copy link
Contributor

LGTM, I agree that this is somewhat preferable to the approach in #3993

ResetEndpoints is useful when the there is a scheduled cluster
changes or when manually manage the cluster without auto-sync
enabled.
For CoreOS users, they will get a updated version of etcdctl without updating
the etcd server version. And the users cannot really control this behavior.
We do not want to suddenly break them without enough communication.

So we still want the most basic opeartions like get, set, watch of etcdctl2 work
with etcd 0.4. This patches solve the incompability issue.
xiang90 added a commit that referenced this pull request Dec 21, 2015
@xiang90 xiang90 merged commit 2681137 into etcd-io:master Dec 21, 2015
@xiang90 xiang90 deleted the ctl_04 branch December 21, 2015 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants