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.

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

fix(cli): project delete empty s3 buckets before deleting #714

Merged

Conversation

iamhopaul123
Copy link
Contributor

@iamhopaul123 iamhopaul123 commented Mar 5, 2020

fix #700, with minor refactoring.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@iamhopaul123 iamhopaul123 requested review from kohidave, efekarakus, bvtujo, SoManyHs and a team March 5, 2020 01:45
@iamhopaul123 iamhopaul123 force-pushed the project-delete/empty-s3-buckets branch from c8514cc to e0bcecb Compare March 5, 2020 17:56
@iamhopaul123 iamhopaul123 force-pushed the project-delete/empty-s3-buckets branch from e0bcecb to c221c19 Compare March 6, 2020 01:39
@@ -52,3 +60,46 @@ func (s *Service) PutArtifact(bucket, fileName string, data io.Reader) (string,

return resp.Location, nil
}

// EmptyBucket deletes all objects within the bucket.
func (s *Service) EmptyBucket(bucket 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.

Looks tricky, thanks for tackling this.

Have you looked at using the BatchDelete of the s3Manager?

Here's a blog post on how it might help simplify things.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried that method before. Ugh it is helpful when using non-versioning bucket. However, when it comes to "versioning bucket", things can be very complicated and "s3manager" they don't support versioning bucket very well, and BatchDelete cannot delete all the other versioning for the objects nor the "delete marker" for that.

Had a discussion with Efe yesterday on those resolved comments above on that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, sorry then!

Copy link
Contributor

Choose a reason for hiding this comment

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

lol no problem!

@iamhopaul123 this looks so much neater and also correct :P thanks for looking into it

@iamhopaul123 iamhopaul123 force-pushed the project-delete/empty-s3-buckets branch from c221c19 to db50373 Compare March 6, 2020 21:55
Copy link
Contributor

@efekarakus efekarakus left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀 with a question below and small nit

_, err = s.s3Svc.DeleteObjects(&s3.DeleteObjectsInput{
Bucket: aws.String(bucket),
Delete: &s3.Delete{
Objects: objectsToDelete,
Copy link
Contributor

Choose a reason for hiding this comment

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

listResp.DeleteMarkers + listResp.Versions can not be more than 1000 objects right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but objectsToDelete shall not exceed 1000 either, because the list return values limit already includes both version and delete marker

Copy link
Contributor

Choose a reason for hiding this comment

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

ok perfect!

@@ -52,3 +60,46 @@ func (s *Service) PutArtifact(bucket, fileName string, data io.Reader) (string,

return resp.Location, nil
}

// EmptyBucket deletes all objects within the bucket.
func (s *Service) EmptyBucket(bucket 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.

lol no problem!

@iamhopaul123 this looks so much neater and also correct :P thanks for looking into it

@@ -143,3 +143,7 @@ type wsProjectManager interface {
type artifactPutter interface {
PutArtifact(bucket, fileName string, data io.Reader) (string, error)
}

type bucketEmptier interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

💩

@iamhopaul123 iamhopaul123 force-pushed the project-delete/empty-s3-buckets branch from db50373 to d2e595a Compare March 7, 2020 00:43
@iamhopaul123 iamhopaul123 merged commit 2d948b7 into aws:master Mar 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update project delete to empty s3 buckets before deleting
3 participants