Skip to content
This repository was archived by the owner on Mar 23, 2026. It is now read-only.

S3: Move Tagging Functionality into Provider Methods#13657

Merged
aidehn merged 9 commits intomainfrom
aidehn/feat/s3-rgta-integration
Feb 4, 2026
Merged

S3: Move Tagging Functionality into Provider Methods#13657
aidehn merged 9 commits intomainfrom
aidehn/feat/s3-rgta-integration

Conversation

@aidehn
Copy link
Copy Markdown
Contributor

@aidehn aidehn commented Jan 27, 2026 •
edited
Loading

Changes

  • Moves tagging functionality for S3 and S3 Control to a provider method so that it can be overwritten by a Pro implementation.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 27, 2026 •
edited
Loading

Test Results - Preflight, Unit

23 114 tests  ±0   21 255 ✅ ±0   6m 5s ⏱️ -11s
     1 suites ±0    1 859 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit e0d6cbb. ± Comparison against base commit 2a499fd.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 27, 2026 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 13m 11s ⏱️ - 48m 25s
2 064 tests  - 3 125  1 886 ✅  - 2 912  178 💤  - 213  0 ❌ ±0 
2 066 runs   - 3 125  1 886 ✅  - 2 912  180 💤  - 213  0 ❌ ±0 

Results for commit e0d6cbb. ± Comparison against base commit 2a499fd.

This pull request removes 3125 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 27, 2026 •
edited
Loading

Test Results (amd64) - Acceptance

7 tests  ±0   5 ✅ ±0   2m 58s ⏱️ -1s
1 suites ±0   2 💤 ±0 
1 files   ±0   0 ❌ ±0 

Results for commit e0d6cbb. ± Comparison against base commit 2a499fd.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 27, 2026 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files  ±    0      5 suites  ±0   1h 36m 30s ⏱️ - 1h 2m 47s
2 088 tests  - 3 522  1 914 ✅  - 3 135  174 💤  - 387  0 ❌ ±0 
2 094 runs   - 3 522  1 914 ✅  - 3 135  180 💤  - 387  0 ❌ ±0 

Results for commit e0d6cbb. ± Comparison against base commit 2a499fd.

This pull request removes 3522 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 28, 2026 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

    2 files      2 suites   8m 10s ⏱️
  559 tests   507 ✅  52 💤 0 ❌
1 118 runs  1 014 ✅ 104 💤 0 ❌

Results for commit e0d6cbb.

♻️ This comment has been updated with latest results.

@aidehn aidehn added semver: patch Non-breaking changes which can be included in patch releases docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes labels Jan 29, 2026
@aidehn aidehn requested a review from viren-nadkarni January 29, 2026 15:45
@aidehn aidehn marked this pull request as ready for review January 29, 2026 15:45
@aidehn aidehn requested review from bentsku and k-a-il as code owners January 29, 2026 15:45
bentsku
bentsku approved these changes Feb 1, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM! Nice work, and great catch about us not removing the bucket tags when removing the bucket 👍

I only have 2 nits, one about naming but it's just a matter of personal preference and does not have to be addressed, and the other one about keeping the validation in the provider operation. Second one would make sense to address 👍

Nice work again!

localstack-core/localstack/services/s3/provider.py
Comment on lines +610 to +612
self._remove_all_bucket_tags(
s3_bucket.bucket_arn, context.account_id, s3_bucket.bucket_region
)
Copy link
Copy Markdown
Contributor

@bentsku bentsku Jan 29, 2026

Choose a reason for hiding this comment

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

awesome catch 👑

@bentsku bentsku added semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases semver: patch Non-breaking changes which can be included in patch releases and removed semver: patch Non-breaking changes which can be included in patch releases semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Feb 1, 2026
@aidehn aidehn force-pushed the aidehn/feat/s3-rgta-integration branch from b00de5b to 6608832 Compare February 4, 2026 07:31
@aidehn aidehn requested a review from bentsku February 4, 2026 07:33
bentsku
bentsku approved these changes Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

LGTM, looking great! Thanks for addressing the comments, nice cleanup 🧹

@aidehn aidehn merged commit bb27e3e into main Feb 4, 2026
49 checks passed
@aidehn aidehn deleted the aidehn/feat/s3-rgta-integration branch February 4, 2026 12:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@viren-nadkarni viren-nadkarni Awaiting requested review from viren-nadkarni

@k-a-il k-a-il Awaiting requested review from k-a-il k-a-il is a code owner

1 more reviewer

@bentsku bentsku bentsku approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

docs: skip Pull request does not require documentation changes notes: skip Pull request does not have to be mentioned in the release notes 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.

2 participants

@aidehn @bentsku