GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — KMS: Move Tagging Functionality into methods on the Provider by aidehn · Pull Request #13595 · localstack/localstack

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

Conversation

@aidehn

@aidehn aidehn commented Jan 7, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@github-actions

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
Comment thread localstack-core/localstack/services/kms/provider.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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

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

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM 👍

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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

@viren-nadkarni viren-nadkarni left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread 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):

@viren-nadkarni viren-nadkarni Jan 27, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread localstack-core/localstack/services/kms/provider.py
Comment on lines +1530 to +1531
if not (tag_keys := request.get("TagKeys", [])):
return

@viren-nadkarni viren-nadkarni Jan 27, 2026

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@bentsku bentsku Jan 30, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

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 😄

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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 added 6 commits February 4, 2026 20:45
@aidehn
POC part 1
7058a97
@aidehn
fix tests
4cd80de
@aidehn
create validate tag list util
673120c
@aidehn
refactor tagging functionality into provider methods
4bb7b1d
@aidehn
format
55dc9b3
@aidehn
add type hint for method
42cc595
@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

Uh oh!

There was an error while loading. Please reload this page.
@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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@aidehn @viren-nadkarni @bentsku @k-a-il
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.