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

rfc: feature flags #4925

Merged
merged 6 commits into from
Nov 11, 2019
Merged

rfc: feature flags #4925

merged 6 commits into from
Nov 11, 2019

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Nov 8, 2019

Design proposal for a pattern/mechanism that will allow us to introduce
breaking capabilities which will only be applied to new projects created
by "cdk init" and won't break old projects without explicit action from the user.


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

Design proposal for a pattern/mechanism that will allow us to introduce
breaking capabilities which will only be applied to new projects created
by "cdk init" and won't break old projects without explicit action from the user.
@mergify
Copy link
Contributor

mergify bot commented Nov 8, 2019

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

design/feature-flags.md Show resolved Hide resolved

We considered an alternative of "bundling" new capabilities under a single flag
that specifies the CDK version which created the project, but this means that
users won't have the ability to pick and choose which future capabilities they
Copy link
Contributor

Choose a reason for hiding this comment

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

Not exactly true, the 2 alternatives could be combined, as I described on the other PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but it feels like over-designing this without having sufficient evidence that it's required. This is not a one-way-door. We can always add "feature groups" later which will enable a whole bunch of feature flags with a single switch (and it could be a version number if we decide it's useful).

Let's start with the basic granular and simple mechanism and then evolve it as need arises, ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new under a new section for "future development"

design/feature-flags.md Show resolved Hide resolved
default (so existing projects will not be affected) but enabled automatically
for new projects created through `cdk init`.

> The name "**future flags**" is a wordplay on "feature flags" (dah!) to
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think people won't have to deal with this enough that the wordplay is worth it. To my mind, it's only confusing. But I don't feel strongly about it either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, changed to "feature flags".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clipping my wings JK

design/feature-flags.md Outdated Show resolved Hide resolved
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@eladb eladb changed the title rfc: future flags rfc: feature flags Nov 8, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

and add a few ideas from the PR
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@SomayaB SomayaB added the contribution/core This is a PR that came from AWS. label Nov 8, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@eladb eladb requested a review from rix0rrr November 11, 2019 09:36
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I still don't like the aesthetics of the proposed solution, and I think it's wrong to burden all future projects with backwards compatibility cruft (even if it's only 1 or 2 flags). Also not looking forward to fielding all the "what is this flag in my configuration doing?" customer questions, given that the answer will be some form of "just leave it there, it's not really intended to be changed(*)"

However, I know you're antsing to get this out and I don't want to hold up progress, so I will approve.

(*) And yes, I know that technically users have the opportunity to change it if they want to, but they're really not supposed to. The NEW behavior is the one we want to encourage and we only do it by flag because we're scared of breaking current users. So effectively we're stuffing 99% of user's configs full of flags they can look at but shouldn't really be touching.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@eladb
Copy link
Contributor Author

eladb commented Nov 11, 2019

@rix0rrr as I said, I am not against introducing a version-based or group-based mechanism on top of this. There is no contradiction. Let's see how this behaves and iterate.

@eladb eladb merged commit db50ab0 into master Nov 11, 2019
@eladb eladb deleted the benisrae/rfc-future-flags branch November 11, 2019 11:15
eladb pushed a commit that referenced this pull request Nov 11, 2019
…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
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

eladb pushed a commit that referenced this pull request Nov 11, 2019
…feature flag) (#4895)

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.
eladb pushed a commit that referenced this pull request Nov 13, 2019
eladb pushed a commit that referenced this pull request Nov 13, 2019
@eladb eladb mentioned this pull request Jan 3, 2020
9 tasks
eladb pushed a commit to cdklabs/decdk that referenced this pull request Jan 18, 2022
…feature flag) (#4895)

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](aws/aws-cdk#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: aws/aws-cdk#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants