Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants