-
-
Notifications
You must be signed in to change notification settings - Fork 51
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 cleanup flag to delete deactivated key if specified #6
Conversation
Note that the key that is used to delete the key was deactivated just above the deletion. It is probably fine as I think it takes a couple of seconds for the key deletion to propagate. But should probably be tested a bunch of times to see that it works reliably. |
@stefansundin apparently if I re-read newly created
Will try to add the check + retry before doing this and see if it helps and how much time does it take for the new key to be available. |
I think what you have is probably ok, just wanted to make sure it was tested thoroughly. You could also just delete the old key and skip disabling it when |
fb76e65
to
dadb849
Compare
@stefansundin I polished a bit previous solution according to what we discussed in this PR. |
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.
I only have one last comment, but it's minor so I am also approving. :)
@@ -24,7 +24,10 @@ Usage of aws-rotate-key: | |||
The profile to use. (default "default") | |||
-version | |||
Print version number (1.0.3) | |||
-y Automatic "yes" to prompts. | |||
-y |
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.
My only comment is that this text is the output from aws-rotate-key -help
, which now will say:
-d Delete old key without deactivation.
-profile string
The profile to use. (default "default")
-version
Print version number (1.0.3)
-y Automatic "yes" to prompts.
It is not obvious that the text is from there. And I don't think we need to keep it exactly the same.
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.
Thanks @ymatsiuk 👍
@ymatsiuk your changes were released with https://github.com/Fullscreen/aws-rotate-key/releases/tag/v1.0.4 |
As requested in #4