Skip to content

EC2: generate security group ids using id manager concept #12494

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
Apr 9, 2025
Merged

EC2: generate security group ids using id manager concept #12494

merged 3 commits into from
Apr 9, 2025

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented Apr 7, 2025 •
edited by cloutierMat
Loading

Motivation

The PR fixing tag behaviour in LocalStack via #12459 broke the LocalStack replicator functionality as the tags that were replicated no longer matched.

Changes

  • Use the moto id generator functionality to generate a security group id from either the ID cache (moto/localstack pro concept) or custom tags

Testing

  • expanded on existing tests to included creating resources with the id manager and ensure it will not break if vpc id are provided

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label Apr 7, 2025
@simonrw simonrw self-assigned this Apr 7, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 7, 2025 •
edited
Loading

LocalStack Community integration with Pro

 2 files  ±    0   2 suites  ±0   39s ⏱️ - 1h 50m 17s
27 tests  - 4 298  25 ✅  - 3 956  2 💤  - 342  0 ❌ ±0 
29 runs   - 4 298  25 ✅  - 3 956  4 💤  - 342  0 ❌ ±0 

Results for commit 3cb2e03. ± Comparison against base commit 72ed3cd.

This pull request removes 4302 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.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[False-id_manager]
tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[False-tag]
tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[True-id_manager]
tests.aws.services.ec2.test_ec2.TestEc2Integrations ‑ test_create_security_group_with_custom_id[True-tag]

♻️ This comment has been updated with latest results.

@cloutierMat cloutierMat marked this pull request as ready for review April 7, 2025 22:10
viren-nadkarni
viren-nadkarni approved these changes Apr 9, 2025
View reviewed changes
Copy link
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.

Looks good, thanks for fixing this 👍

@cloutierMat cloutierMat merged commit 163790c into master Apr 9, 2025
33 checks passed
@cloutierMat cloutierMat deleted the fix/ec2-security-group-ident-handling branch April 9, 2025 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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

@macnev2013 macnev2013 Awaiting requested review from macnev2013 macnev2013 is a code owner

Assignees

@simonrw simonrw

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
@simonrw @viren-nadkarni @cloutierMat