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

Add revoke command #58

Merged
merged 1 commit into from
Oct 3, 2018
Merged

Add revoke command #58

merged 1 commit into from
Oct 3, 2018

Conversation

namreg
Copy link
Contributor

@namreg namreg commented Aug 26, 2018

Resolve #43.
Any comments are appreciated.

@CLAassistant
Copy link

CLAassistant commented Aug 26, 2018

CLA assistant check
All committers have signed the CLA.

@mcpherrinm
Copy link
Contributor

Thanks! I'll review soon. It looks fine from a high level overview

@mcpherrinm
Copy link
Contributor

Sorry this has hung around for so long (the last month has been busy).

The only real concern I have with this PR is a lack of tests. I'm not too worried about that for now, but would appreciate at least a basic functional test.

note that we just merged another PR moving away from Godep and this will require your PR to be updated too.

@namreg
Copy link
Contributor Author

namreg commented Sep 21, 2018

@mcpherrinm Got it! I will add some tests asap and let you know when done.

@namreg
Copy link
Contributor Author

namreg commented Sep 23, 2018

@mcpherrinm I have done

@mcpherrinm
Copy link
Contributor

One more nit: You've check in the certstrap binary. Please remove it from your PR (including from history) so as to not bloat this repo.

It would be nice if there was an option on the revoke command to revoke by serial (in case you don't have the cert file), but we can merge without that and add it in the future.

Otherwise I think we can merge.

@namreg
Copy link
Contributor Author

namreg commented Oct 3, 2018

@mcpherrinm I've removed certstrap binary, also rebased on master and squashed commits.

@mcpherrinm mcpherrinm merged commit e27060a into square:master Oct 3, 2018
@mcpherrinm
Copy link
Contributor

Great! Thank you for your contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants