Skip to content

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented May 10, 2023

This PR adds test to check the behaviour and the fix of getmoto/moto#6295

This PR builds on #8326

This will confirm the fix for #8183

@bentsku bentsku self-assigned this May 10, 2023
@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels May 10, 2023
@bentsku bentsku changed the title add test for LegalHold when targeting object version add test for S3 LegalHold when targeting object version May 12, 2023
@bentsku bentsku force-pushed the fix-s3-lock-version branch from cba82e1 to 3af808c Compare May 17, 2023 12:22
@bentsku bentsku changed the base branch from master to bump-moto-4.1.9post1 May 17, 2023 12:23
@coveralls
Copy link

coveralls commented May 17, 2023

Coverage Status

Coverage: 82.256% (-0.002%) from 82.258% when pulling 545b737 on fix-s3-lock-version into 4b7a6f2 on master.

@github-actions
Copy link

github-actions bot commented May 17, 2023

LocalStack Community integration with Pro

2 062 tests   1 779 ✔️  1h 24m 52s ⏱️
       2 suites     283 💤
       2 files           0

Results for commit 545b737.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review May 17, 2023 16:44
@bentsku bentsku requested a review from macnev2013 as a code owner May 17, 2023 16:44
Base automatically changed from bump-moto-4.1.9post1 to master May 19, 2023 09:06
@@ -3887,6 +3887,89 @@ def test_s3_sse_default_kms_key(
response = aws_client.s3.get_object(Bucket=bucket_1, Key=key_name)
snapshot.match("get-obj-default-kms-s3-key-from-bucket", response)

@pytest.mark.aws_validated
@pytest.mark.skip_snapshot_verify(paths=["$..ServerSideEncryption"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we skipping to verify the path ServerSideEncryption ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because AWS now has server side encryption by default on all newly created buckets: https://docs.aws.amazon.com/AmazonS3/latest/userguide/default-encryption-faq.html

This behaviour is not yet in moto or LocalStack, so we need to skip the return value for now. All newly created tests now have to have this skip until we fix it 😞

Copy link
Contributor

@macnev2013 macnev2013 left a comment

Choose a reason for hiding this comment

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

Have one question about snapshot else LGTM.

Copy link
Contributor

@macnev2013 macnev2013 left a comment

Choose a reason for hiding this comment

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

Have one question about snapshot else LGTM.

Copy link
Contributor

@macnev2013 macnev2013 left a comment

Choose a reason for hiding this comment

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

Have one question about snapshot else LGTM.

@bentsku bentsku force-pushed the fix-s3-lock-version branch from 3af808c to 1ffab12 Compare May 22, 2023 09:49
@bentsku bentsku force-pushed the fix-s3-lock-version branch from 1ffab12 to 545b737 Compare May 22, 2023 11:10
@bentsku bentsku merged commit 8af4f8f into master May 22, 2023
@bentsku bentsku deleted the fix-s3-lock-version branch May 22, 2023 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service 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.

bug: S3 (AccessDenied) while trying to remove the file from the versioned bucket with legal hold locking enabled
3 participants