Support Deregistration Protection #516
Conversation
There was a problem hiding this comment.
Hey @phuhung273,
Thanks for the PR! I very much appreciate you adding an acceptance test at the same time, thanks a lot for this; I'll try it in our environment to be sure, but this looks good to me at first glance.
I've left a few comments on the code and the docs, the options for the deregistration_protection are not visible in the READMEs for the builders, so we should probably address that before we merge and release this feature.
Besides that, LGTM! Thanks for the PR again, much appreciated
| } | ||
|
|
||
| for region, ami := range amis { | ||
| ui.Say(fmt.Sprintf("Enabling deregistration protection on AMI (%s) in region %q ...", ami, region)) |
There was a problem hiding this comment.
You can use ui.Sayf instead of a ui.Say(fmt.Sprintf, though I would suggest we demote this to a log.Printf, this is mostly useful for debugging, showing that log for each AMI to enable protection on might be noisy a bit
There was a problem hiding this comment.
Changed to log.Printf. I actually stole your code here
There was a problem hiding this comment.
Ah yeah, ui.Sayf is a reasonably recent addition to the SDK, it's likely we have some old code that still uses ui.Say(fmt.Sprintf around the codebase.
We should probably clean this up someday!
builder/ebs/builder_acc_test.go
Outdated
| return fmt.Errorf("Failed to find ami %s at region %s", ami.Name, ami.Region) | ||
| } | ||
|
|
||
| ec2conn, _ := testEC2Conn(ami.Region) |
There was a problem hiding this comment.
I would suggest catching the error here and returning it, just so we are aware if creating the connection fails for some reason, otherwise the next statement will panic, which will not make it easier for someone to later troubleshoot.
There was a problem hiding this comment.
Thank you so much. Error catched. Should have realised it.
| @@ -137,4 +137,7 @@ | |||
| If you specify a value for seconds, Amazon EC2 rounds the seconds to the nearest minute. | |||
| You can’t specify a date in the past. The upper limit for DeprecateAt is 10 years from now. | |||
|
|
|||
| - `deregistration_protection` (DeregistrationProtectionOptions) - See [DeregistrationProtectionOptions](#deregistration-protection-options) below for more | |||
There was a problem hiding this comment.
Since this doesn't unroll the contents of the generated docs, I would suggest manually adding them to the docs for each builder, ideally under a # Deregistration Protection Options section so the fragment can point to it.
There was a problem hiding this comment.
I just added to docs/builders. But cannot find a way to view on local machine. Hope it is correct.
As new contributor, im really grateful for your kind instruction @lbajolet-hashicorp |
There was a problem hiding this comment.
Thanks for the reroll @phuhung273
The code looks good to me, thanks for addressing all my comments; in the current state the PR should be ready to merge today.
I'll just run the acceptance tests you added locally to make sure this works as advertised, and if it's green, we can trigger the merge today!
The acceptance test added as part of the deregistration protection PR did not include a region as part of the AMI created by the test, so we add it in order for the test to succeed.
The AMI we create for the deregistration test would linger after the test was executed, which is less than idea, so this commit ensures we do remove the AMI after the test executes.
|
Hi again @phuhung273, I've just tried to run the acceptance test, which failed because the With those changes, the test does succeed and cleans-up afterwards, so provided the tests go green now, I'll be able to trigger a merge! |
Thank you so much @lbajolet-hashicorp. Not sure why the test succeeded on my side, maybe because I already Again, thank you so much for being incredibly helpful and giving instruction. Hope it wont cause any problem for the plugin. |
Closes #486
Description
aws-sdk-go v1.51.32for EnableDeregistrationProtection APIIntegration Test