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

feat: future flags 'core:enableStackNameDuplicates', 'aws-secretsmanager:parseOwnedSecretName' and 'aws-kms:defaultKeyPolicies' are no longer supported #12644

Merged
merged 20 commits into from
Feb 15, 2021

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Jan 21, 2021

The following feature flags have been marked as expired -

  • @aws-cdk/core:enableStackNameDuplicates
  • @aws-cdk/aws-secretsmanager:parseOwnedSecretName
  • @aws-cdk/aws-kms:defaultKeyPolicies

These flags will be unavailable in CDKv2 and setting this flag in the context
will result in an error.

Most of the changes here are related to the feature flag
'@aws-cdk/aws-kms:defaultKeyPolicies' which changes the KMS
permissions. Since KMS is a fundamental AWS service, used by almost all
downstream services, there is a significant change to all snapshots.


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

@nija-at nija-at self-assigned this Jan 21, 2021
@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Jan 21, 2021
@gitpod-io
Copy link

gitpod-io bot commented Jan 21, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 21, 2021
@mergify
Copy link
Contributor

mergify bot commented Jan 21, 2021

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

mergify bot pushed a commit that referenced this pull request Jan 22, 2021
…ags (#12645)

CDKv1 introduced feature flags that allowed users to opt-in to new
backwards-incompatible behaviour. There are two types of flags -
one set that need to be entirely removed in CDKv2 and their
'enabled' behaviour made the default, and another set where the
the new behaviour should be made the default, and allow users to
explicitly opt-out.

This change addresses the testing strategy for the former set (and
not the latter). There exist unit tests that test the behaviour both
when the feature flags are enabled and feature flags and disabled.
However, in v2, the feature flags will be removed and its usage
blocked. The default behaviour will be as if the feature flag is
enabled.

Introduce branching logic that treats these unit tests depending on
whether it's executed in the context of CDKv1 or CDKv2.

The logic is as follows:
- In the context of v1, all tests will execute as normal.
- In the context of v2, tests that rely on feature flag to be unset will
   not be executed.
- In the context of v2, tests that rely on feature flag to be set will
  not configure the feature flag on the App's context and continue to
  execute the test.

This logic has been captured at a single location in two methods -
`testFeatureFlag()` and `testFeatureFlagDisabled()`.

To validate this logic, the unit tests that rely on the feature flags
`aws-kms:defaultKeyPolicies`,
`aws-secretsmanager:parseOwnedSecretName` and
`core:enableStackNameDuplicates` have been updated to use
these methods.

Forward looking: The final PR in v2 is expected to look like
this - #12644

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
mohanrajendran pushed a commit to mohanrajendran/aws-cdk that referenced this pull request Jan 24, 2021
…ags (aws#12645)

CDKv1 introduced feature flags that allowed users to opt-in to new
backwards-incompatible behaviour. There are two types of flags -
one set that need to be entirely removed in CDKv2 and their
'enabled' behaviour made the default, and another set where the
the new behaviour should be made the default, and allow users to
explicitly opt-out.

This change addresses the testing strategy for the former set (and
not the latter). There exist unit tests that test the behaviour both
when the feature flags are enabled and feature flags and disabled.
However, in v2, the feature flags will be removed and its usage
blocked. The default behaviour will be as if the feature flag is
enabled.

Introduce branching logic that treats these unit tests depending on
whether it's executed in the context of CDKv1 or CDKv2.

The logic is as follows:
- In the context of v1, all tests will execute as normal.
- In the context of v2, tests that rely on feature flag to be unset will
   not be executed.
- In the context of v2, tests that rely on feature flag to be set will
  not configure the feature flag on the App's context and continue to
  execute the test.

This logic has been captured at a single location in two methods -
`testFeatureFlag()` and `testFeatureFlagDisabled()`.

To validate this logic, the unit tests that rely on the feature flags
`aws-kms:defaultKeyPolicies`,
`aws-secretsmanager:parseOwnedSecretName` and
`core:enableStackNameDuplicates` have been updated to use
these methods.

Forward looking: The final PR in v2 is expected to look like
this - aws#12644

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@nija-at nija-at force-pushed the nija-at/feature-flags branch 3 times, most recently from 17d1e7e to 26eb2c3 Compare February 9, 2021 15:08
@nija-at nija-at force-pushed the nija-at/feature-flags branch from bbde447 to 6ed820c Compare February 11, 2021 17:27
@nija-at nija-at force-pushed the nija-at/feature-flags branch from 6ed820c to f96aa4a Compare February 11, 2021 17:29
@nija-at nija-at changed the title (do not merge) reset feature flags for v2 feat: feature flags 'core:enableStackNameDuplicates', 'aws-secretsmanager:parseOwnedSecretName' and 'aws-kms:defaultKeyPolicies' are no longer supported Feb 11, 2021
@nija-at nija-at changed the title feat: feature flags 'core:enableStackNameDuplicates', 'aws-secretsmanager:parseOwnedSecretName' and 'aws-kms:defaultKeyPolicies' are no longer supported feat: future flags 'core:enableStackNameDuplicates', 'aws-secretsmanager:parseOwnedSecretName' and 'aws-kms:defaultKeyPolicies' are no longer supported Feb 11, 2021
@nija-at nija-at marked this pull request as ready for review February 15, 2021 11:32
@nija-at nija-at requested a review from njlynch February 15, 2021 11:32
@nija-at nija-at marked this pull request as draft February 15, 2021 11:32
@nija-at nija-at requested a review from rix0rrr February 15, 2021 11:32
@nija-at nija-at marked this pull request as ready for review February 15, 2021 11:35
@nija-at nija-at removed the pr/do-not-merge This PR should not be merged at this time. label Feb 15, 2021
@nija-at nija-at added the pr-linter/exempt-readme The PR linter will not require README changes label Feb 15, 2021
@nija-at nija-at force-pushed the nija-at/feature-flags branch from 1703d8c to aa79d42 Compare February 15, 2021 14:04
@mergify
Copy link
Contributor

mergify bot commented Feb 15, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 7bb3144
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Feb 15, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 7554246 into v2-main Feb 15, 2021
@mergify mergify bot deleted the nija-at/feature-flags branch February 15, 2021 15:05
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. pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants