Skip to content

Add tests for KMS resource tagging and untagging #12121

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
merged 3 commits into from
Jan 20, 2025
Merged

Add tests for KMS resource tagging and untagging #12121

merged 3 commits into from
Jan 20, 2025

Conversation

k-a-il
Copy link
Contributor

@k-a-il k-a-il commented Jan 10, 2025

Motivation

The AWS-validated snapshot KMS tests for tagging and untagging keys are currently missing. We need to add these tests, collect their responses, and verify them against LocalStack.

Changes

  • AWS-validated tests for tagging and untagging keys have been added for the KMS service.

@k-a-il k-a-il added the semver: patch Non-breaking changes which can be included in patch releases label Jan 10, 2025
@k-a-il k-a-il requested a review from bentsku January 10, 2025 10:09
@k-a-il k-a-il self-assigned this Jan 10, 2025
@k-a-il k-a-il requested a review from sannya-singal as a code owner January 10, 2025 10:09
@localstack-bot
Copy link
Contributor

localstack-bot commented Jan 10, 2025 •
edited
Loading

All contributors have signed the CLA ✍️ ✅
Posted by the CLA Assistant Lite bot.

localstack-bot
localstack-bot reviewed Jan 10, 2025
View reviewed changes
Copy link
Contributor

@localstack-bot localstack-bot left a comment

Choose a reason for hiding this comment

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

Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.

@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 10, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   4m 18s ⏱️ - 1h 47m 26s
598 tests  - 3 354  588 ✅  - 3 054   9 💤  - 300  1 ❌ ±0 
600 runs   - 3 354  588 ✅  - 3 054  11 💤  - 300  1 ❌ ±0 

For more details on these failures, see this check.

Results for commit b486e6d. ± Comparison against base commit 4832016.

This pull request removes 3358 and adds 4 tests. Note that renamed tests count towards both. 
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]
…

tests.aws.services.kms.test_kms.TestKMS ‑ test_create_key_with_tag_and_untag
tests.aws.services.kms.test_kms.TestKMS ‑ test_tag_existing_key_and_untag
tests.aws.services.kms.test_kms.TestKMS ‑ test_untag_key_partially
tests.aws.services.kms.test_kms.TestKMS ‑ test_update_and_add_tags_on_tagged_key

♻️ This comment has been updated with latest results.

@k-a-il
Copy link
Contributor Author

k-a-il commented Jan 10, 2025

I have read the CLA Document and I hereby sign the CLA

localstack-bot added a commit that referenced this pull request Jan 10, 2025
@localstack-bot
@k-a-il has signed the CLA in #12121
8eb4e3a
bentsku
bentsku reviewed Jan 11, 2025
View reviewed changes
Copy link
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.

Really nice! This looks really great, and I like the way the tests are structured 👌 well done!

I think we're still missing one case from the deleted test (which is very not clear by the way, thanks for updating that one!). Once the comment is addressed, I believe we're good to merge! Can't wait to see the follow up 🚀 Congrats!

@k-a-il k-a-il force-pushed the kms-tests-tag-untag-keys branch from 6eadee6 to b486e6d Compare January 17, 2025 12:49
bentsku
bentsku approved these changes Jan 20, 2025
View reviewed changes
Copy link
Contributor

@bentsku bentsku left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! Thanks for addressing the comment! Welcome to LocalStack's contributors 🥳
The test failure is unrelated to the changes (it is in S3), so this is safe to merge.

@k-a-il k-a-il merged commit 7e62817 into master Jan 20, 2025
26 of 30 checks passed
@k-a-il k-a-il deleted the kms-tests-tag-untag-keys branch January 20, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@localstack-bot localstack-bot localstack-bot left review comments

@bentsku bentsku bentsku approved these changes

@sannya-singal sannya-singal Awaiting requested review from sannya-singal

Assignees

@k-a-il k-a-il

Labels
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.
3 participants
@k-a-il @localstack-bot @bentsku