-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Remove old etcd client dependency #11610
Conversation
func GetAndTestEtcdClient(etcdClientInfo configapi.EtcdConnectionInfo) (*etcdclient.Client, error) { | ||
etcdClient, err := EtcdClient(etcdClientInfo) | ||
func GetAndTestEtcdClient(etcdClientInfo configapi.EtcdConnectionInfo) (newetcdclient.Client, error) { | ||
etcdClient, err := MakeNewEtcdClient(etcdClientInfo) |
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.
@smarterclayton does this change seems correct to you?
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.
Rename newetcdclient to just etcdclient
.
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.
renamed.
[test] |
The vendor commit change needs to be renamed to UPSTREAM: : or UPSTREAM: coreos/go-etcd: drop. |
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.
One question, but overall this LGTM
@@ -23,48 +20,17 @@ import ( | |||
// connect to the etcd server and block until the server responds at least once, or return an | |||
// error if the server never responded. | |||
// TODO: switch this function to use EtcdHelper. |
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.
Is this TODO still valid?
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.
don't think so.
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.
removed.
41b22ed
to
b627197
Compare
for part 1 removal of old, lgtm. You'll need to abstract the client itself on another iteration, b/c that's the v2 client. |
b627197
to
a336ad0
Compare
@timothysc @smarterclayton @soltysh updated, PTAL. |
a336ad0
to
3f1b9d6
Compare
Still LGTM, but you need to update your commit message to match one of these. |
a294838
to
7f8b478
Compare
lgtm |
You missed a spot :) On Mon, Oct 31, 2016 at 10:37 AM, OpenShift Bot notifications@github.com
|
@smarterclayton hrm I had to rewrite the etcd dumper :-) please take a look if I did it right... |
Looks fine. |
It looks like you forgot to update tests, both integration and extended with your changes. |
6595d89
to
a35102a
Compare
@soltysh I don't, this is go 1.7 (on my dev) vs. go 1.6 (in CI) :-) |
Oh yeah, that happens... |
a35102a
to
68020fd
Compare
This was hopefully last hit of #11661. re-[test] |
Flake #11649. |
re-[test] |
flake: #11799 [test] |
Evaluated for origin test up to 68020fd |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11186/) (Base Commit: b81b382) |
tests are green, shipping! [merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/11186/) (Base Commit: b81b382) (Image: devenv-rhel7_5326) |
Evaluated for origin merge up to 68020fd |
Also updates master_config to use new etcd client.
Fixes: #11573