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.

Sign up for GitHub

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

cloud: allow to call S3 cleanup via defer #1145

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

cloud: allow to call S3 cleanup via defer #1145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions pkg/cloud/awscloud/awscloud.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,10 +294,7 @@ func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch str

// we no longer need the object in s3, let's just delete it
logrus.Infof("[AWS] 🧹 Deleting image from S3: %s/%s", bucket, key)
_, err = a.s3.DeleteObject(&s3.DeleteObjectInput{
Bucket: aws.String(bucket),
Key: aws.String(key),
})
err = a.Cleanup(name, bucket, key)
if err != nil {
return nil, nil, err
}
Expand Down Expand Up @@ -388,6 +385,18 @@ func (a *AWS) Register(name, bucket, key string, shareWith []string, rpmArch str
return registerOutput.ImageId, snapshotID, nil
}

// Cleanup removes any temporary resources created during the image registration process.
// Make sure to call this function via defer to ensure cleanup is always performed.
// It will return an error if resources no longer exist or if there was an error deleting them.
func (a *AWS) Cleanup(name, bucket, key string) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we call this "DeleteObject" as this is what it is doing? Why is "name" passed in? AFAICT it is not used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I saw an interface in bib codebase, I got impression there is an idea to have a common uploader interface at some point. Azure also has a cleanup similar to S3, tho, in that case this is completely done by a caller.

Idiomatic Go is to create an interface called Cleaner with this only method and then clients can use that to perform cleaning. I dontt mind changing this to anything else, but there is Closer in the standard library as well as Reader as well. For DeleteObject we would need to call this Deleter. But what if in the future cleaning involves more steps not just deleting "objects"?

_, err := a.s3.DeleteObject(&s3.DeleteObjectInput{
Bucket: aws.String(bucket),
Key: aws.String(key),
})

return err
}

// target region is determined by the region configured in the aws session
func (a *AWS) CopyImage(name, ami, sourceRegion string) (string, error) {
result, err := a.ec2.CopyImage(
Expand Down
1 change: 1 addition & 0 deletions pkg/cloud/gcp/compute.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ func GuestOsFeaturesByDistro(distroName string) []*computepb.GuestOsFeature {
// The image must be RAW image named 'disk.raw' inside a gzip-ed tarball.
//
// To delete the Storage object (image) used for the image import, use StorageObjectDelete().
// Make sure to use defer in order to delete objects when error occurs.
//
// bucket - Google storage bucket name with the uploaded image archive
// object - Google storage object name of the uploaded image
Expand Down
Loading