Skip to content

implement string replacement feature in CFn v2 #12911

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
simonrw merged 8 commits into from
Aug 5, 2025
Merged

implement string replacement feature in CFn v2 #12911

simonrw merged 8 commits into from
Aug 5, 2025

Conversation

@pinzon
Copy link
Member

@pinzon pinzon commented Jul 25, 2025 •
edited
Loading

Motivation

This PR implements the capability to replace AWS URLs in CFn templates for CFn v2.

Changes

  • Override visit_terminal_value_created to replace strings if necessary

Testing

  • more unskipped tests

Notes

  • Possibly needs to be implemented in visit_terminal_value_updated too.
  • Consider moving the code to the Preproc.

@pinzon pinzon added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Jul 25, 2025
@github-actions
Copy link

github-actions bot commented Jul 25, 2025 •
edited
Loading

Test Results - Preflight, Unit

22 063 tests  ±0   20 329 ✅ ±0   6m 28s ⏱️ ±0s
     1 suites ±0    1 734 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit 86bae28. ± Comparison against base commit b411538.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jul 25, 2025 •
edited
Loading

Test Results (amd64) - Acceptance

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

Results for commit 86bae28. ± Comparison against base commit b411538.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jul 25, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 42m 15s ⏱️ - 2m 0s
4 603 tests ±0  4 176 ✅ ±0  427 💤 ±0  0 ❌ ±0 
4 605 runs  ±0  4 176 ✅ ±0  429 💤 ±0  0 ❌ ±0 

Results for commit 86bae28. ± Comparison against base commit b411538.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Jul 25, 2025 •
edited
Loading

Test Results (amd64) - Integration, Bootstrap

    5 files      5 suites   2h 21m 1s ⏱️
4 962 tests 4 383 ✅ 579 💤 0 ❌
4 968 runs  4 383 ✅ 585 💤 0 ❌

Results for commit 86bae28.

♻️ This comment has been updated with latest results.

@pinzon pinzon marked this pull request as ready for review July 28, 2025 14:26
@pinzon pinzon added this to the Playground milestone Jul 28, 2025
simonrw
simonrw requested changes Jul 29, 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 adding this, I think your assessment is correct we need to update visit_terminal_value_modified but I think we only need to transform the after value.

Can you try to create a test that exercises this so we can find out for sure? I think it will require modifying the URLs in the lambda test, which should not be too difficult

@simonrw simonrw modified the milestones: Playground, 4.8 Jul 29, 2025
@localstack-bot
Copy link
Contributor

localstack-bot commented Jul 31, 2025

Currently, only patch changes are allowed on main. Your PR labels (semver: minor) indicate that it cannot be merged into the main at this time.

@pinzon pinzon requested a review from simonrw July 31, 2025 21:17
@pinzon pinzon force-pushed the cp/cfn/v2/string-replacement branch from 4587fcf to bc62600 Compare August 1, 2025 15:37
@pinzon pinzon force-pushed the cp/cfn/v2/string-replacement branch from a196917 to 9c2d468 Compare August 4, 2025 20:07
@simonrw
Copy link
Contributor

simonrw commented Aug 5, 2025

@pinzon I added some updates namely expanding on the lambda test to cover the update phase, and then needed to update the code in visit_terminal_value_unchanged and visit_terminal_value_modified

@simonrw simonrw merged commit 7015841 into main Aug 5, 2025
40 checks passed
@simonrw simonrw deleted the cp/cfn/v2/string-replacement branch August 5, 2025 14:20
@simonrw simonrw mentioned this pull request Sep 5, 2025
Merged
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

semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Milestone

4.8

Development

Successfully merging this pull request may close these issues.

4 participants

@pinzon @localstack-bot @simonrw