Skip to content

refactoring of removal of AWS::NoValue in reference list for CFn #12288

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 2 commits into from
Feb 25, 2025
Merged

refactoring of removal of AWS::NoValue in reference list for CFn #12288

merged 2 commits into from
Feb 25, 2025

Conversation

pinzon
Copy link
Member

@pinzon pinzon commented Feb 19, 2025 •
edited
Loading

Motivation

As discovered by a customer. CFn should remove the AWS::NoValue from list of references when pasing such list as as an Attribute Value for a resource definition.

Example. In the following code, CFn should remove the AWS::NoValue from the list of RouteTableIds before attempting to create the resource.

S3VpcEndpoint:
    Type: AWS::EC2::VPCEndpoint
    Properties:
      RouteTableIds:
        - Fn::If:
            - cNewVpc
            - Ref: PrivateSubnet1RouteTable
            - Ref: AWS::NoValue
        - Fn::If:
            - cNewVpc
            - Ref: PrivateSubnet2RouteTable
            - Ref: AWS::NoValue

The previous solution (#12163) only managed to fix this for Fn::Join list parameter. With the new changes, the fix is applied to any referenced list.

Changes

  • Temployer utils removes the AWS::NoValue from the lists.
  • The Fn:Join procces now uses the cleaned result instead of filtering it itself

No testing

  • Since it's a refactoring a green pipeline should be enough.
  • Most of the resources that directly accept a list as attribute are available only in PRO

@pinzon pinzon added aws:cloudformation AWS CloudFormation semver: patch Non-breaking changes which can be included in patch releases labels Feb 19, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 19, 2025 •
edited
Loading

LocalStack Community integration with Pro

  2 files  ±    0    2 suites  ±0   21m 19s ⏱️ - 1h 31m 59s
422 tests  - 3 679  311 ✅  - 3 457  111 💤  - 222  0 ❌ ±0 
424 runs   - 3 679  311 ✅  - 3 457  113 💤  - 222  0 ❌ ±0 

Results for commit 5a21a8f. ± Comparison against base commit 5d463b5.

This pull request removes 3679 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.

@pinzon pinzon marked this pull request as ready for review February 19, 2025 22:00
@simonrw
Copy link
Contributor

simonrw commented Feb 20, 2025

You mention in your PR description:

Since it's a refactoring a green pipeline should be enough.

Only if we have examples where we use this functionality in CFn templates, right?

Most of the resources that directly accept a list as attribute are available only in PRO

Let's make sure we do a manual run of the pro tests to be sure

@pinzon pinzon added this to the 4.2 milestone Feb 21, 2025
@pinzon pinzon force-pushed the cloudformation/refact/template_deployer_no_value_in_list branch from 44100c8 to f8b5d1a Compare February 21, 2025 19:05
simonrw
simonrw approved these changes Feb 24, 2025
View reviewed changes
Copy link
Contributor

@simonrw simonrw left a comment

Choose a reason for hiding this comment

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

Thanks for proving the change is valid in -ext. This change looks good to me, a nice simplification stripping out the values in the list type before it makes it to the intrinsic function call!

localstack-core/localstack/services/cloudformation/engine/template_deployer.py Outdated
@@ -756,8 +753,10 @@ def _resolve_refs_recursively(
{inner_list[0]: inner_list[1]},
)

# remove _aws_no_value_ from resulting references
clean_list = []
for i in range(len(value)):
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this no longer needs to be a list over the indices

Suggested change
for i in range(len(value)):
for item in value:

localstack-core/localstack/services/cloudformation/engine/template_deployer.py
@@ -756,8 +753,10 @@ def _resolve_refs_recursively(
{inner_list[0]: inner_list[1]},
)

# remove _aws_no_value_ from resulting references
clean_list = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for writing the longer (but easier to understand) loop manually building up a new list, rather than some horribly complex list comprehension!

@pinzon pinzon merged commit 958b4f1 into master Feb 25, 2025
31 checks passed
@pinzon pinzon deleted the cloudformation/refact/template_deployer_no_value_in_list branch February 25, 2025 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonrw simonrw simonrw approved these changes

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

Assignees
No one assigned
Labels
aws:cloudformation AWS CloudFormation semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.
2 participants
@pinzon @simonrw