8000 fixed s3 deletion from stack in cdk by macnev2013 · Pull Request #11700 · localstack/localstack · GitHub
[go: up one dir, main page]

Skip to content

fixed s3 deletion from stack in cdk #11700

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

Merged
merged 2 commits into from
Oct 22, 2024
Merged

fixed s3 deletion from stack in cdk #11700

merged 2 commits into from
Oct 22, 2024

Conversation

macnev2013
Copy link
Contributor

Motivation

This PR aims to fix the S3 bucket cleaning when a stack contains more than one buckets (with objects in it). According to my theory due to late binding in the lambda function, the cleanup_s3_bucket is not able to get the reference to the actual variable.

Changes

  • updated the cleanup of s3 buckets in the cdc provisioner function.

@macnev2013 macnev2013 added the semver: patch Non-breaking changes which can be included in patch releases label Oct 16, 2024
Copy link
github-actions bot commented Oct 16, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 41m 27s ⏱️ +14s
3 508 tests ±0  3 095 ✅ ±0  413 💤 ±0  0 ❌ ±0 
3 510 runs  ±0  3 095 ✅ ±0  415 💤 ±0  0 ❌ ±0 

Results for commit 1b8f9b7. ± Comparison against base commit 4ad57db.

♻️ This comment has been updated with latest results.

@macnev2013 macnev2013 marked this pull request as ready for review October 16, 2024 17:04
Copy link
Member
@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

Great catch! And it is indeed because of the late binding here where the actual value of s3_bucket was only resolved at the time of the lambda function execution, rather than the definition.

@@ -243,9 +243,10 @@ def provision(self, skip_deployment: Optional[bool] = False):
]

for s3_bucket in s3_buckets:
bucket_to_delete = s3_bucket # Capture current value
Copy link
Member

Choose a reason for hiding this comment

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

FYI: this line doesn't actually do anything in regards to the root cause, it would still lead to the same issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. I've removed this line.

lambda bucket=bucket_to_delete: cleanup_s3_bucket(
self.aws_client.s3, bucket, delete_bucket=False
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
lambda bucket=bucket_to_delete: cleanup_s3_bucket(
self.aws_client.s3, bucket, delete_bucket=False
lambda s3_bucket=s3_bucket: cleanup_s3_bucket(
self.aws_client.s3, s3_bucket, delete_bucket=False

this alone would be enough to bind the value here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've update the lambda to use the loop variable.

@macnev2013 macnev2013 force-pushed the fix/cdk-provisioner-s3 branch from 60d796a to 1b8f9b7 Compare October 21, 2024 13:21
@macnev2013 macnev2013 merged commit 99eec4c into master Oct 22, 2024
34 checks passed
@macnev2013 macnev2013 deleted the fix/cdk-provisioner-s3 branch October 22, 2024 06:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikschubert dominikschubert dominikschubert approved these changes

@steffyP steffyP Awaiting requested review from steffyP steffyP is a code owner

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0