Skip to content
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.

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

Refactor handling of CFn stack parameters #8322

Merged
merged 8 commits into from Jun 6, 2023

Conversation

dominikschubert
Copy link
Member

@dominikschubert dominikschubert commented May 16, 2023

Changes

  • Parameters are resolved only once when executing CREATE/UPDATE tasks. While the corresponding logic lives in parameters.py now, the responsibility is moved further up to the API provider to make this as transparent as possible.
  • Unifies parameter resolving (previously this was done in multiple places such as in Stack methods, template_processing, transformations, template deployer)
  • Update handling is now more robust and simpler.

What hasn't changed

  • Does not affect how parameters are used when resolving values/refs in the TemplateDeployer/Stack, i.e. they are still merged with resources and mappings/conditions in Stack.resources for now. This will be split up in a future PR.
  • No new parameter types have been added

Also note I didn't add further tests yet since I didn't want to bloat this too much. I've started on more parity focused tests but I want to give them a bit more polish and build a proper parameter test suite out of it first.

@dominikschubert dominikschubert self-assigned this May 16, 2023
@dominikschubert dominikschubert added the semver: patch Non-breaking changes which can be included in patch releases label May 16, 2023
@github-actions
Copy link

github-actions bot commented May 16, 2023

LocalStack Community integration with Pro

2 084 tests   1 803 ✔️  1h 27m 40s ⏱️
       2 suites     281 💤
       2 files           0

Results for commit 463a380.

♻️ This comment has been updated with latest results.

@dominikschubert dominikschubert force-pushed the refactor-cfn-resolve-parameters branch from 2781ff7 to 1277eda Compare May 22, 2023 07:47
@@ -1,40 +1,8 @@
import re
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't see much of a value at this point for these resolve unit tests since I'd prefer to use proper integration tests here soon.

@coveralls
Copy link

coveralls commented May 22, 2023

Coverage Status

coverage: 82.732% (+0.01%) from 82.721% when pulling 463a380 on refactor-cfn-resolve-parameters into 30f2123 on master.

@dominikschubert dominikschubert marked this pull request as ready for review May 23, 2023 04:56
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.

Excited to move this piece of the process to its proper place - before the template deployer gets created 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

It's nice to not have to deal with parameters in the deployer itself!

@dominikschubert dominikschubert force-pushed the refactor-cfn-resolve-parameters branch 2 times, most recently from 4b492d2 to 0916800 Compare May 31, 2023 22:36
@dominikschubert dominikschubert force-pushed the refactor-cfn-resolve-parameters branch from 0916800 to 463a380 Compare June 5, 2023 14:45
@dominikschubert dominikschubert changed the title Refactor CFn parameter resolving Refactor handling of CFn stack parameters Jun 6, 2023
@dominikschubert dominikschubert merged commit fdf2f2c into master Jun 6, 2023
26 checks passed
@dominikschubert dominikschubert deleted the refactor-cfn-resolve-parameters branch June 6, 2023 13:00
@dfangl dfangl added this to the 2.2 milestone Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants