Closed phuhung273 closed 1 week ago
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 theREADME
s 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
As new contributor, im really grateful for your kind instruction @lbajolet-hashicorp
Hi again @phuhung273,
I've just tried to run the acceptance test, which failed because the Region
was not specified in the AMI specification for the test.
I've also noticed that the created AMI would linger after testing, so I took the liberty to push a couple commits to address those shortcomings on top of yours, hope this is not a problem.
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!
Hi again @phuhung273,
I've just tried to run the acceptance test, which failed because the
Region
was not specified in the AMI specification for the test. I've also noticed that the created AMI would linger after testing, so I took the liberty to push a couple commits to address those shortcomings on top of yours, hope this is not a problem.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 set AWS_REGION
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.32
for EnableDeregistrationProtection APIIntegration Test