fix(core): impossible to use the same physical stack name for two stacks (under feature flag) #4895

eladb · 2019-11-07T09:52:00Z

Since we used the stack name as the template file name, if users wanted to use the
same stack name in two environments, the emitted templates overwrote each other.

Apparently, the CLI used the artifact ID as the stack name, so this became a bit more complex.
This means that assembly.getStack() is now ambiguous, so I renamed it getStackByName which fails if there are two stacks with the same name (legitimate) and added getStackArtifact which uses the artifact ID.

The core library will effecitvely generate identical cloud assemblies if the stack name and artifact IDs are the same, and to ensure backwards compat, no existing tests have been changed (albeit it would have been more correct to replace all getStackByName with getStackArtifact, but effectively this is the same thing if they are equal).

We want the template file name to use the artifact ID instead of the physical stack name but this can break users that depend on this behavior (despite the fact that it's a formal API). To avoid this, we will only enable this new behavior behind a feature flag which means that it will only be enabled for new projects created through cdk init, but old projects will still get the old behavior.

RFC for feature flags: #4925

Fixes #4412

BREAKING CHANGE: synthesized template file names for new projects created by cdk init will use the stack artifact ID instead of the physical stack name to enable multiple stacks to use the same name (in most cases artifact id and stack name are the same). This is enabled by inserting the feature flag @aws-cdk/core:enableStackNameDuplicates under context in newly generated cdk.json files.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

mergify · 2019-11-07T09:54:05Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-07T10:01:13Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/@aws-cdk/core/test/test.stack.ts

aws-cdk-automation · 2019-11-07T22:09:58Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/@aws-cdk/assert/lib/inspector.ts

aws-cdk-automation · 2019-11-07T22:41:30Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-08T09:01:09Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-08T09:24:23Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-08T09:26:42Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-08T09:46:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr

Why does this PR not include bump to the CX protocol?

CONTRIBUTING.md

packages/@aws-cdk/assert/lib/inspector.ts

packages/@aws-cdk/core/lib/stack.ts

packages/@aws-cdk/core/test/test.stack.ts

packages/aws-cdk/bin/cdk.ts

packages/aws-cdk/lib/api/deploy-stack.ts

aws-cdk-automation · 2019-11-08T10:39:25Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-08T10:41:08Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-08T14:05:17Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-11T10:10:45Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…feature flag) Since we used the stack name as the template file name, if users wanted to use the same stack name in two environments, the emitted templates overwrote each other. Furthermore, the CLI used the artifact ID as the stack name, so this became a bit more complex. This means that `assembly.getStack()` is now ambiguous, so I renamed it `getStackByName` which fails if there are two stacks with the same name (legitimate) and added `getStackArtifact` which uses the artifact ID. The core library will effectively generate identical cloud assemblies if the stack name and artifact IDs are the same, and to ensure backwards compat, no existing tests have been changed (albeit it would have been more correct to replace all `getStackByName` with `getStackArtifact`, but effectively this is the same thing if they are equal). We want the template file name to use the artifact ID instead of the physical stack name but this can break users that depend on this behaviour (despite the fact that it's a formal API). To avoid this, we will only enable this new behaviour behind a [feature flag](#4925) which means that it will only be enabled for new projects created through `cdk init`, but old projects will still get the old behaviour. RFC for feature flags: #4925 Fixes #4412 BREAKING CHANGE: template file names in `cdk.out` for new projects created by `cdk init` will use `stack.artifactId` instead of the physical stack name to enable multiple stacks to use the same name. In most cases the artifact ID is the same as the stack name. To enable this fix for old projects, add the context key `@aws-cdk/core:enableStackNameDuplicates: true` in your `cdk.json` file.

aws-cdk-automation · 2019-11-11T11:41:03Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-11T11:47:24Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-11T11:49:41Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

joekiller · 2019-11-11T19:05:27Z

@eladb

FWIW I had to update my classes that implement core.Stack because I just happened to be using the property artifact_id in my stacks and this change was breaking for me.

    self.artifact_id = artifact_id
    AttributeError: can't set attribute

Legacy mode for #4895 still used the uniquely generated id instead of the stack name as the artifact ID in the cloud assembly. The implications were that even if users were not opted-in to the new behavior (through the feature flag), they could not use the stack name in the CLI because the stack artifact ID was still new. This fix ensures that if the feature flag is not enabled, the artifact ID itself uses the stack name, hence allowing users to query by stack name as long as they are not opted in to the new behavior. Fixes #4997

…4998) Legacy mode for #4895 still used the uniquely generated id instead of the stack name as the artifact ID in the cloud assembly. The implications were that even if users were not opted-in to the new behavior (through the feature flag), they could not use the stack name in the CLI because the stack artifact ID was still new. This fix ensures that if the feature flag is not enabled, the artifact ID itself uses the stack name, hence allowing users to query by stack name as long as they are not opted in to the new behavior. Fixes #4997

nija-at reviewed Nov 7, 2019

View reviewed changes

packages/@aws-cdk/core/test/test.stack.ts Show resolved Hide resolved

eladb added the pr/do-not-merge This PR should not be merged at this time. label Nov 7, 2019

eladb requested review from RomainMuller and SoManyHs as code owners November 7, 2019 22:08

eladb commented Nov 7, 2019

View reviewed changes

packages/@aws-cdk/assert/lib/inspector.ts Show resolved Hide resolved

eladb changed the title ~~fix(core): unable to use the same stack name for two stacks in different environments~~ fix(core): impossible to use the same physical stack name for two stacks Nov 8, 2019

eladb changed the title ~~fix(core): impossible to use the same physical stack name for two stacks~~ fix(core): impossible to use the same physical stack name for two stacks (behind future flag) Nov 8, 2019

eladb changed the title ~~fix(core): impossible to use the same physical stack name for two stacks (behind future flag)~~ fix(core): impossible to use the same physical stack name for two stacks (future flag) Nov 8, 2019

eladb removed the pr/do-not-merge This PR should not be merged at this time. label Nov 8, 2019

rix0rrr reviewed Nov 8, 2019

View reviewed changes

eladb changed the title ~~fix(core): impossible to use the same physical stack name for two stacks (future flag)~~ fix(core): impossible to use the same physical stack name for two stacks (under feature flag) Nov 8, 2019

SomayaB added the contribution/core This is a PR that came from AWS. label Nov 8, 2019

SomayaB assigned eladb Nov 8, 2019

eladb force-pushed the benisrae/fix-template-file-name branch from d567535 to 36f0545 Compare November 11, 2019 11:26

eladb merged commit 658f100 into master Nov 11, 2019

eladb deleted the benisrae/fix-template-file-name branch November 11, 2019 11:45

eladb mentioned this pull request Nov 13, 2019

fix(core): unable to find stack by name using the cli in legacy mode #4998

Merged

konstantinj mentioned this pull request Jan 16, 2020

Having stacks in different regions with the same name (again) does not work #5832

Closed

eladb mentioned this pull request Jan 19, 2020

Stack names are not honoured when creating a nested stack #5853

Closed

eladb mentioned this pull request Jun 24, 2020

Expose nodeFactory in construct-compat #7892

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): impossible to use the same physical stack name for two stacks (under feature flag) #4895

fix(core): impossible to use the same physical stack name for two stacks (under feature flag) #4895

eladb commented Nov 7, 2019 •

edited

Loading

mergify bot commented Nov 7, 2019

aws-cdk-automation commented Nov 7, 2019

aws-cdk-automation commented Nov 7, 2019

aws-cdk-automation commented Nov 7, 2019

aws-cdk-automation commented Nov 8, 2019

aws-cdk-automation commented Nov 8, 2019

aws-cdk-automation commented Nov 8, 2019

aws-cdk-automation commented Nov 8, 2019

rix0rrr left a comment

aws-cdk-automation commented Nov 8, 2019

aws-cdk-automation commented Nov 8, 2019

aws-cdk-automation commented Nov 8, 2019

aws-cdk-automation commented Nov 11, 2019

aws-cdk-automation commented Nov 11, 2019

aws-cdk-automation commented Nov 11, 2019

aws-cdk-automation commented Nov 11, 2019

joekiller commented Nov 11, 2019

fix(core): impossible to use the same physical stack name for two stacks (under feature flag) #4895

fix(core): impossible to use the same physical stack name for two stacks (under feature flag) #4895

Conversation

eladb commented Nov 7, 2019 • edited Loading

mergify bot commented Nov 7, 2019

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 7, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

rix0rrr left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 8, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 11, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 11, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 11, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 11, 2019

AWS CodeBuild CI Report

joekiller commented Nov 11, 2019

eladb commented Nov 7, 2019 •

edited

Loading