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

chore: enable feature flags on all integ tests #13110

Merged
merged 5 commits into from
Feb 17, 2021

Conversation

nija-at
Copy link
Contributor

@nija-at nija-at commented Feb 17, 2021

Most feature flags will be expired in CDKv2 and their 'enabled'
behaviour made the default.
To keep the divergence between the two branches (master and
v2-main) to a minimum, update all integration tests to have the
flags enabled.

If the flag needs to be different for any specific test, it can be
overridden when constructing the App object.

To keep the set of changes small, only the following flags have
been added. The rest will be added in a subsequent change.

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

Currently, only these three feature flags are expired in CDKv2.
Fixing them first will make the branch divergence smaller and
reduce already occuring merge conflicts.


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 Feb 17, 2021
@gitpod-io
Copy link

gitpod-io bot commented Feb 17, 2021

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 17, 2021
@nija-at nija-at force-pushed the nija-at/featureflags-integtests branch from 4db52ec to d2d8803 Compare February 17, 2021 14:04
@nija-at nija-at marked this pull request as ready for review February 17, 2021 14:07
@nija-at nija-at requested review from njlynch, a team and eladb February 17, 2021 14:07
@@ -337,7 +337,11 @@ export const DEFAULT_SYNTH_OPTIONS = {
},
],
},
// Enable feature flags for all integ tests
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to simply enable all feature flags? (i.e. iterate over the keys and add them all, same as we do in the CLI when a new project is created).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eventually yes. See PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, this is in cdk-integ-tools and the list of feature flags is in cx-api. It would not be possible to obtain the list here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I see that cdk-integ-tools already depends on cx-api. I'll update this accordingly in a subsequent PR.

@mergify
Copy link
Contributor

mergify bot commented Feb 17, 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: 54a06be
  • 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 17, 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 200d52e into master Feb 17, 2021
@mergify mergify bot deleted the nija-at/featureflags-integtests branch February 17, 2021 15:57
NovakGu pushed a commit to NovakGu/aws-cdk that referenced this pull request Feb 18, 2021
Most feature flags will be expired in CDKv2 and their 'enabled'
behaviour made the default.
To keep the divergence between the two branches (`master` and
`v2-main`) to a minimum, update all integration tests to have the
flags enabled.

If the flag needs to be different for any specific test, it can be
overridden when constructing the `App` object.

To keep the set of changes small, only the following flags have
been added. The rest will be added in a subsequent change.
- @aws-cdk/aws-kms:defaultKeyPolicies
- @aws-cdk/core:enableStackNameDuplicates
- @aws-cdk/aws-secretsmanager:parseOwnedSecretName

Currently, only these three feature flags are expired in CDKv2.
Fixing them first will make the branch divergence smaller and
reduce already occuring [merge conflicts].

[merge conflicts]: aws#13099

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
eladb pushed a commit that referenced this pull request Feb 22, 2021
Most feature flags will be expired in CDKv2 and their 'enabled'
behaviour made the default.
To keep the divergence between the two branches (`master` and
`v2-main`) to a minimum, update all integration tests to have the
flags enabled.

If the flag needs to be different for any specific test, it can be
overridden when constructing the `App` object.

To keep the set of changes small, only the following flags have
been added. The rest will be added in a subsequent change.
- @aws-cdk/aws-kms:defaultKeyPolicies
- @aws-cdk/core:enableStackNameDuplicates
- @aws-cdk/aws-secretsmanager:parseOwnedSecretName

Currently, only these three feature flags are expired in CDKv2.
Fixing them first will make the branch divergence smaller and
reduce already occuring [merge conflicts].

[merge conflicts]: #13099

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.

4 participants