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

Jump to bottom

fix S3 s3_empty_bucket fixture and add parity #9099

Merged
merged 2 commits into from Sep 11, 2023
Merged

fix S3 s3_empty_bucket fixture and add parity #9099

merged 2 commits into from Sep 11, 2023

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Sep 8, 2023 •
edited

Motivation

It seems I introduced an issue when fixing the s3_empty_bucket fixture to force deleting locked objects.

2023-09-08T17:33:09.910 DEBUG --- [  MainThread] localstack.testing.pytest.fixtures : error cleaning up bucket test-bucket-5c896954: An error occurred (InvalidArgument) when calling the DeleteObjects operation: x-amz-bypass-governance-retention is only applicable to Object Lock enabled buckets.

It also showed that LocalStack didn't have parity when checking this, and allowed the parameter to be passed if the bucket doesn't have it enabled. Added the fix in LocalStack as well, but this was an issue only when running against AWS (lots of clean up to do 😭)

Changes

  • added a check in the fixture to check if the bucket can be locked.
  • added a test for the fixture, so we're sure it's working.
  • added parity in LocalStack to also raise the error if the parameter is used.

@bentsku bentsku added the semver: patch Non-breaking changes which can be included in patch releases label Sep 8, 2023
@bentsku bentsku self-assigned this Sep 8, 2023
@bentsku bentsku added the aws:s3 Amazon Simple Storage Service label Sep 8, 2023
@bentsku bentsku changed the base branch from master to enable-new-s3-tests September 8, 2023 16:54
@bentsku bentsku marked this pull request as ready for review September 8, 2023 16:58
@bentsku bentsku requested review from viren-nadkarni and removed request for calvernaz September 8, 2023 16:58
viren-nadkarni
viren-nadkarni approved these changes Sep 11, 2023
View reviewed changes
Copy link
Member

@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

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

💯

Base automatically changed from enable-new-s3-tests to master September 11, 2023 09:49
@coveralls
Copy link

Coverage Status

coverage: 79.727% (+0.06%) from 79.672% when pulling 95a140e on fix-s3-fixture into 75a078d on master.

@github-actions
Copy link

LocalStack Community integration with Pro

       2 files         2 suites   1h 23m 19s ⏱️
2 174 tests 1 691 ✔️ 483 💤 0 ❌
2 175 runs  1 691 ✔️ 484 💤 0 ❌

Results for commit 95a140e.

dominikschubert
dominikschubert approved these changes Sep 11, 2023
View reviewed changes
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 🚀

@bentsku bentsku merged commit f9945cd into master Sep 11, 2023
26 checks passed
@bentsku bentsku deleted the fix-s3-fixture branch September 11, 2023 12:59
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

@viren-nadkarni viren-nadkarni viren-nadkarni approved these changes

@macnev2013 macnev2013 Awaiting requested review from macnev2013

Assignees

@bentsku bentsku

Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@bentsku @coveralls @dominikschubert @viren-nadkarni