-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Nuke User owned AMIs #18
Conversation
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.
LGTM overall! Just some minor comments.
} | ||
|
||
_, err := svc.DeregisterImage(params) | ||
if err != nil { |
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.
If you try to delete an AMI that's actively in use, do you get an error? If so, we should catch it, log it, and keep going.
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.
Will try this out
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.
From my tests an AMI can be deleted even when there exists an EC2 instance that was created with it. I believe once the VM is provisioned with that machine image there's no link between them and that ami exists just for re-provisioning new EC2 instances
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.
Good to know!
Ship it 👍
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.
Sweet, I'll merge once I fix the test
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.
@tonerdo Is deleting an AMI actually a two-step procedure as I had to do in ec2-snapper? At that time, I first had to deregister the AMI (as you do here), and second, had to delete the AMI's corresponding EBS snapshot.
|
||
func createTestAMI(t *testing.T, session *session.Session, name string) ec2.Image { | ||
svc := ec2.New(session) | ||
instance := createTestEC2Instance(t, session, name, false) |
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.
Should this defer
the deletion of this instance?
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.
It does the defer on line 62. If the defer
is done here, the EC2 instance could have been nuked before we get a chance to create the Image. The safest bet is to just clean up after the test function itself
This PR does the following: