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

Convert the output of App.synth rather than the construct tree. #18

Closed
pgavlin opened this issue Apr 20, 2022 · 1 comment · Fixed by #167
Closed

Convert the output of App.synth rather than the construct tree. #18

pgavlin opened this issue Apr 20, 2022 · 1 comment · Fixed by #167
Assignees
Labels
kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Milestone

Comments

@pgavlin
Copy link
Member

pgavlin commented Apr 20, 2022

Just what it says on the tin. This should make for a more faithful conversion of the output of synthesis, and may make it more straightforward to support assets and other CDK features.

@pgavlin pgavlin added the kind/enhancement Improvements or new features label Apr 20, 2022
@corymhall corymhall mentioned this issue May 28, 2024
6 tasks
@corymhall corymhall self-assigned this Sep 30, 2024
corymhall added a commit that referenced this issue Oct 1, 2024
Previously this library walked the construct tree in-memory and called the
internal `_toCloudFormation()` method each `Cfn*` construct has to get
the CloudFormation resource fragment. This is both inefficient (because
we call `synth` which internally does the same thing, and then we do it
again), and potentially inaccurate (`synth` internally does a lot more
than we do when we walk the construct tree).

This PR is a refactor (maybe closer to a rewrite) that switches to
taking the output of `app.synth()` and processing the resulting
CloudAssembly. The CloudAssembly has everything necessary to do the
conversion and contains the fully resolved CloudFormation template (we don't
have to worry about unresolved tokens!).

This also sets us up for future work like nested stacks, multiple
stacks, and #153.

Note to reviewers. This is a pretty big refactor so I would recommend
reviewing this instead as a restart rather than trying to figure out
what the old code is doing.

Closes #18
@mjeffryes mjeffryes added this to the 0.111 milestone Oct 2, 2024
@pulumi-bot pulumi-bot added the resolution/fixed This issue was fixed label Oct 7, 2024
@pulumi-bot
Copy link
Contributor

This issue has been addressed in PR #167 and shipped in release v1.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants