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

KMS: Move Tagging Functionality into methods on the Provider#13595

Merged
aidehn merged 6 commits intomainfrom
aidehn/feat/kms-rgta-integration
Feb 5, 2026
Merged

KMS: Move Tagging Functionality into methods on the Provider#13595
aidehn merged 6 commits intomainfrom
aidehn/feat/kms-rgta-integration

Conversation

@aidehn
Copy link
Copy Markdown
Contributor

@aidehn aidehn commented Jan 7, 2026 •
edited
Loading

Changes

  • Moves validation logic to validate_tag_list to be reused.
  • Moves tagging logic to methods on the KMS Provider class in order to be flexible and can be overwritten in Pro without duplicating code.

@github-actions
Copy link
Copy Markdown

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

Test Results - Preflight, Unit

23 114 tests  ±0   21 255 ✅ ±0   6m 36s ⏱️ +30s
     1 suites ±0    1 859 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 42cc595. ± Comparison against base commit 7c60109.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

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

LocalStack Community integration with Pro

  2 files    2 suites   5m 9s ⏱️
774 tests 766 ✅  8 💤 0 ❌
776 runs  766 ✅ 10 💤 0 ❌

Results for commit 42cc595.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

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

Test Results (amd64) - Acceptance

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

Results for commit 42cc595. ± Comparison against base commit 7c60109.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Copy Markdown

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

Test Results (amd64) - Integration, Bootstrap

  5 files    5 suites   17m 30s ⏱️
798 tests 790 ✅  8 💤 0 ❌
804 runs  790 ✅ 14 💤 0 ❌

Results for commit 42cc595.

♻️ 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 8, 2026
@aidehn aidehn force-pushed the aidehn/feat/kms-rgta-integration branch 2 times, most recently from 247c199 to 494a487 Compare January 13, 2026 13:18
@aidehn aidehn changed the title PoC: KMS RGTA Integration KMS: Move Tagging Functionality into methods on the Provider Jan 13, 2026
@aidehn aidehn marked this pull request as ready for review January 13, 2026 15:00
@aidehn aidehn requested a review from k-a-il as a code owner January 13, 2026 15:00
@aidehn aidehn requested a review from viren-nadkarni January 13, 2026 15:00
k-a-il
k-a-il suggested changes Jan 15, 2026
View reviewed changes
@aidehn aidehn force-pushed the aidehn/feat/kms-rgta-integration branch from 494a487 to fa88094 Compare January 15, 2026 11:33
@k-a-il k-a-il self-requested a review January 15, 2026 17:22
k-a-il
k-a-il approved these changes Jan 15, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@k-a-il k-a-il left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@aidehn aidehn added this to the 4.14 milestone Jan 16, 2026
@aidehn aidehn force-pushed the aidehn/feat/kms-rgta-integration branch from fa88094 to 3d9bb51 Compare January 26, 2026 10:49
viren-nadkarni
viren-nadkarni approved these changes Jan 27, 2026
View reviewed changes
Copy link
Copy Markdown
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.

LGTM!

localstack-core/localstack/services/kms/provider.py Outdated
def _set_key_tags(self, key: KmsKey, account_id: str, region: str, tags: TagList) -> None:
return

def _remove_key_tags(self, key: KmsKey, account_id: str, region: str, tag_keys: TagKeyList):
Copy link
Copy Markdown
Member

@viren-nadkarni viren-nadkarni Jan 27, 2026

Choose a reason for hiding this comment

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

Suggested change
def _remove_key_tags(self, key: KmsKey, account_id: str, region: str, tag_keys: TagKeyList):
def _remove_key_tags(self, key: KmsKey, account_id: str, region: str, tag_keys: TagKeyList) -> None:

localstack-core/localstack/services/kms/provider.py
Comment on lines +1530 to +1531
if not (tag_keys := request.get("TagKeys", [])):
return
Copy link
Copy Markdown
Member

@viren-nadkarni viren-nadkarni Jan 27, 2026

Choose a reason for hiding this comment

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

Don't use the walrus operator if you intend to use the variable outside the scope of the conditional block.

https://effectivepython.com/2020/02/02/prevent-repetition-with-assignment-expressions

It's not syntactically wrong, but defining the variable outside the conditional block implies that the var is used later.

Suggested change
if not (tag_keys := request.get("TagKeys", [])):
return
tag_keys = request.get("TagKeys", [])
if not tag_keys:
return

Copy link
Copy Markdown
Contributor

@bentsku bentsku Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Interesting, I have been using the walrus operator a lot in that way, TIL. That makes sense in a way!

Edit: I'm not seeing a mention against using the outside the conditional block, or did I miss it?
Edit2: found this as well: https://news.ycombinator.com/item?id=44584016, and people are generally confused by it, so makes sense! I'll try not to use that anymore 😄

@aidehn aidehn force-pushed the aidehn/feat/kms-rgta-integration branch 3 times, most recently from 786fc35 to dd2d3ef Compare January 30, 2026 12:57
@aidehn aidehn force-pushed the aidehn/feat/kms-rgta-integration branch from dd2d3ef to 42cc595 Compare February 4, 2026 12:47
@aidehn aidehn merged commit 0eded68 into main Feb 5, 2026
42 checks passed
@aidehn aidehn deleted the aidehn/feat/kms-rgta-integration branch February 5, 2026 03:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@bentsku bentsku bentsku left review comments

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

@k-a-il k-a-il k-a-il 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

4.14

Development

Successfully merging this pull request may close these issues.

4 participants

@aidehn @viren-nadkarni @bentsku @k-a-il