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/v3: add lease keep-alive --once flag #8775

Merged
merged 2 commits into from
Oct 27, 2017
Merged

Conversation

marcovc
Copy link
Contributor

@marcovc marcovc commented Oct 26, 2017

Fixes: #8719

@marcovc marcovc mentioned this pull request Oct 26, 2017
@xiang90
Copy link
Contributor

xiang90 commented Oct 26, 2017

lgtm. @gyuho do we need an e2e test for it? if yes, can you point @marcovc to the place to add the test?

/cc @jpbetz

@gyuho
Copy link
Contributor

gyuho commented Oct 26, 2017

Yeah, simple e2e test case would be good. We can write one around https://github.com/coreos/etcd/blob/master/e2e/ctl_v3_lease_test.go#L75.

@marcovc
Copy link
Contributor Author

marcovc commented Oct 27, 2017

Sorry, I didn't got it. Am I supposed to write the test? If yes, then should I create a second pull request?

@gyuho
Copy link
Contributor

gyuho commented Oct 27, 2017

@marcovc You can add another commit for the test. Thanks!

@marcovc
Copy link
Contributor Author

marcovc commented Oct 27, 2017

@gyuho, I added the test, although as far as I understand it simply tests if the command runs, and not if it does what is expected. But so does the other tests in that file. I called the "test" binary but I'm not sure the test was called. Let me know if you need anything else.

@gyuho
Copy link
Contributor

gyuho commented Oct 27, 2017

lgtm. defer to @xiang90

@gyuho gyuho changed the title etcdctl v3: adds the --once option to the lease keep-alive command etcdctl/v3: add lease keep-alive --once flag Oct 27, 2017
@xiang90 xiang90 merged commit d75a6a3 into etcd-io:master Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants