Skip to content

validate and add S3Control tests #10932

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
Merged
bentsku merged 3 commits into from
Jun 2, 2024
Merged

validate and add S3Control tests #10932

bentsku merged 3 commits into from
Jun 2, 2024

Conversation

@bentsku
Copy link
Contributor

@bentsku bentsku commented May 30, 2024 •
edited
Loading

Motivation

As part of the S3 Control provider migration, I had started to add more tests with #9730.
Sadly, I didn't have the time to fully go through with the migration, and we didn't get a request yet for it. I'll focus more on it if we have then, it only needs a bit more work which should be fine (+ integration with S3 itself).

But as part of validating and removing aws.unknown test markers, I'm already cherry picking the tests.

Changes

  • validate existing tests for S3 Control
  • add more tests regarding S3 Control resources, but those are skipped for now as we don't implement the logic yet

@bentsku bentsku added the aws:s3control AWS S3 Control label May 30, 2024
@bentsku bentsku self-assigned this May 30, 2024
@bentsku bentsku added the semver: patch Non-breaking changes which can be included in patch releases label May 30, 2024
@bentsku bentsku added this to the 3.5 milestone May 30, 2024
@bentsku bentsku force-pushed the validate-s3-control branch from 0eba42b to adb48c8 Compare May 30, 2024 20:25
@github-actions
Copy link

github-actions bot commented May 30, 2024 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 44m 25s ⏱️ + 3m 46s
3 013 tests +9  2 696 ✅ +1  317 💤 +8  0 ❌ ±0 
3 015 runs  +9  2 696 ✅ +1  319 💤 +8  0 ❌ ±0 

Results for commit 9bb20e8. ± Comparison against base commit c5fdfbe.

This pull request removes 2 and adds 11 tests. Note that renamed tests count towards both. 
tests.aws.services.s3control.test_s3control ‑ test_lifecycle_public_access_block
tests.aws.services.s3control.test_s3control ‑ test_public_access_block_validations

tests.aws.services.s3.test_s3.TestS3 ‑ test_get_object_attributes_with_space
tests.aws.services.s3control.test_s3control.TestLegacyS3Control ‑ test_lifecycle_public_access_block
tests.aws.services.s3control.test_s3control.TestLegacyS3Control ‑ test_public_access_block_validations
tests.aws.services.s3control.test_s3control.TestS3ControlAccessPoint ‑ test_access_point_already_exists
tests.aws.services.s3control.test_s3control.TestS3ControlAccessPoint ‑ test_access_point_bucket_not_exists
tests.aws.services.s3control.test_s3control.TestS3ControlAccessPoint ‑ test_access_point_lifecycle
tests.aws.services.s3control.test_s3control.TestS3ControlAccessPoint ‑ test_access_point_name_validation
tests.aws.services.s3control.test_s3control.TestS3ControlAccessPoint ‑ test_access_point_pagination
tests.aws.services.s3control.test_s3control.TestS3ControlAccessPoint ‑ test_access_point_public_access_block_configuration
tests.aws.services.s3control.test_s3control.TestS3ControlPublicAccessBlock ‑ test_crud_public_access_block
…

♻️ This comment has been updated with latest results.

@bentsku bentsku requested review from cloutierMat and steffyP May 31, 2024 11:54
@bentsku bentsku marked this pull request as ready for review May 31, 2024 11:54
cloutierMat
cloutierMat approved these changes May 31, 2024
View reviewed changes
Copy link
Member

@cloutierMat cloutierMat left a comment

Choose a reason for hiding this comment

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

Nice list of new tests!

Good to see there is still work to keep you busy too! 🤣

Just left one question regarding snapshots but it might not be relevant 😄

tests/aws/services/s3control/test_s3control.py
Comment on lines 104 to 107
assert put_response["ResponseMetadata"]["HTTPStatusCode"] == 200

get_response = s3control_client.get_public_access_block(AccountId=account_id)
assert access_block_config == get_response["PublicAccessBlockConfiguration"]
Copy link
Member

@cloutierMat cloutierMat May 31, 2024

Choose a reason for hiding this comment

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

Question: Was there any issues with performing snapshots? Or reasoning as to why it is not needed?

Copy link
Contributor Author

@bentsku bentsku May 31, 2024

Choose a reason for hiding this comment

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

Oh no, this was a legacy test, I've just validated it but did not add snapshot. I think the goal would be to delete this one in favor of the newer ones created but currently skipped because the parity isn't there at all for now!

Copy link
Member

@cloutierMat cloutierMat May 31, 2024

Choose a reason for hiding this comment

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

👌 makes total sense!

Copy link
Contributor Author

@bentsku bentsku May 31, 2024

Choose a reason for hiding this comment

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

I'll add a comment on the class to clarify still! thanks 😄

@bentsku bentsku merged commit afddd5f into master Jun 2, 2024
@bentsku bentsku deleted the validate-s3-control branch June 2, 2024 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloutierMat cloutierMat cloutierMat approved these changes

@steffyP steffyP Awaiting requested review from steffyP

Assignees

@bentsku bentsku

Labels

aws:s3control AWS S3 Control semver: patch Non-breaking changes which can be included in patch releases

Projects

None yet

Milestone

3.5

Development

Successfully merging this pull request may close these issues.

2 participants

@bentsku @cloutierMat