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

[core] SynthUtils.toCloudFormation(stack) fails on stacks defined in external project #10671

Closed
tobias-bardino opened this issue Oct 3, 2020 · 2 comments · Fixed by #10690
Closed
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort needs-triage This issue or PR still needs to be triaged. p1

Comments

@tobias-bardino
Copy link

SynthUtils.toCloudFormation(stack) fails with Unable to find artifact with id when stack is defined in an external project.

Reproduction Steps

I've created an example to reproduces the error here: https://github.com/gwriss/instanceof-stack-bug

What did you expect to happen?

SynthUtils.toCloudFormation(stack) should spit out a CF stack

What actually happened?

SynthUtils.toCloudFormation(stack) fails with Unable to find artifact with id "MyTestStack291FA567"

All our stack-tests fails after upgrade from 1.54.0 -> 1.65.0 (we have snapshot tests on all external stacks using toCloudFormation)

Environment

  • CLI Version : 1.65.0
  • Framework Version: 1.65.0
  • Node.js Version: v12.18.4
  • OS : OSX Catalina 10.15.7 (19H2)
  • Language (Version): TypeScript (3.9.7)

Other

It seems to be related to #1249 where instanceof Stack does not work as expected and should be replaced with isStack(stack)

It used to work in CDK 1.54.0 (there is a branch in my test-repo where the test runs successfully on 1.54.0)

A search for the instanceof Stack in the repo shows another occurrence. Maybe it would be beneficial to fix that too?

➜ ag "instanceof Stack"
CHANGELOG.md
5101:// must be before defining the table (this instanceof Stack)

packages/@aws-cdk/core/lib/stack-synthesizers/_shared.ts
91:    if (node instanceof Stack && node.nestedStackParent === undefined) {

This is 🐛 Bug Report

@tobias-bardino tobias-bardino added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 3, 2020
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Oct 3, 2020
@tobias-bardino
Copy link
Author

I think this would fix the error but I am not able to write a test for it, because of the requirement of another project (another nodes_modules dir) to reproduce the problem

diff --git a/packages/@aws-cdk/core/lib/private/synthesis.ts b/packages/@aws-cdk/core/lib/private/synthesis.ts
index c8243ec03..e93df721a 100644
--- a/packages/@aws-cdk/core/lib/private/synthesis.ts
+++ b/packages/@aws-cdk/core/lib/private/synthesis.ts
@@ -153,11 +153,11 @@ function synthesizeTree(root: IConstruct, builder: cxapi.CloudAssemblyBuilder) {
       assembly: builder,
     };
 
-    if (construct instanceof Stack) {
+    if (Stack.isStack(construct)) {
       construct.synthesizer.synthesize(session);
     } else if (construct instanceof TreeMetadata) {
       construct._synthesizeTree(session);

rix0rrr added a commit that referenced this issue Oct 5, 2020
Even though this is arguably somewhat of an antipattern and user error:

- You should not be vending `Stack`s from libraries.
- All libraries should be using a single version of the CDK so that
  NPM can dedupe it (or use `peerDependencies` to avoid this altogether)

This is still a sharp edge that we can blunt a little by not using
the `instanceof` language feature.

No test because it needs a second copy of the CDK to expose it.

Fixes #10671.
@rix0rrr rix0rrr added effort/small Small work item – less than a day of effort p1 labels Oct 5, 2020
@mergify mergify bot closed this as completed in #10690 Oct 5, 2020
mergify bot pushed a commit that referenced this issue Oct 5, 2020
#10690)

Even though this is arguably somewhat of an antipattern and user error:

- You should not be vending `Stack`s from libraries.
- All libraries should be using a single version of the CDK so that
  NPM can dedupe it (or use `peerDependencies` to avoid this altogether)

This is still a sharp edge that we can blunt a little by not using
the `instanceof` language feature.

No test because it needs a second copy of the CDK to expose it.

Fixes #10671.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

github-actions bot commented Oct 5, 2020

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. effort/small Small work item – less than a day of effort needs-triage This issue or PR still needs to be triaged. p1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants