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(codepipeline): change default value for crossAccountKeys to false (under feature flag) #28556

Merged

Conversation

go-to-k
Copy link
Contributor

@go-to-k go-to-k commented Jan 3, 2024

The documentation mentions updating the default for CDK v2. Sounds like we should add it in with feature flag.

Closes #28247.


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

@aws-cdk-automation aws-cdk-automation requested a review from a team January 3, 2024 10:06
@github-actions github-actions bot added distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 labels Jan 3, 2024
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@go-to-k
Copy link
Contributor Author

go-to-k commented Jan 3, 2024

Exemption Request: This just changes the default value, so unit tests could cover it.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Jan 3, 2024
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 3, 2024
@GavinZZ GavinZZ self-requested a review January 30, 2024 22:09
Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

Thanks for contributing. This change looks good other than some minor question/changes.

packages/aws-cdk-lib/cx-api/lib/features.ts Show resolved Hide resolved
@@ -52,6 +52,7 @@ const prodStage = {
};

new codepipeline.Pipeline(stack, 'Pipeline', {
crossAccountKeys: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to explicitly set it to true? The feature flag is not enabled by default and shouldn't change existing behaviour right?
Same questions for all other places where you added this prop.

Copy link
Contributor Author

@go-to-k go-to-k Feb 1, 2024

Choose a reason for hiding this comment

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

It appears that the feature flag is enabled by default for integ tests in CodeBuild CI. I put it all back and got an error in CodeBuild CI.

Changes

CodeBuild CI Build log (AccessDenied by Request has expired)

Copy link
Contributor Author

@go-to-k go-to-k Feb 2, 2024

Choose a reason for hiding this comment

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

I still tried to go back to the original code to test it out, and the CI was an error.

fd0ee48

ci1

ci2

However, there is also an integ test where the feature flag remains off by default.

https://github.com/go-to-k/aws-cdk/blob/cba7c75fdd055b1cd48f21fbaa3ab29cf5944693/packages/@aws-cdk-testing/framework-integ/test/aws-cloudwatch-actions/test/integ.lambda-alarm-action.ts#L60-L74

I thought it might be due to a difference in integ.json (this file and the another file) by using new integ.IntegTest() or not, but since I still don't understand how this works, I think I'll leave this PR as is for now. (Because fixing this could be another potentially destructive change.)

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 ran the other test (integ.lambda-alarm-action.ts) as a new integ file (with no existing snapshot) to try it out, and it seems that the feature flag is enabled by default.
(Does using new integ.IntegTest() turn off the feature flag with an existing snapshot?)

Either way, I think we're good to go for now.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 30, 2024
packages/aws-cdk-lib/cx-api/lib/features.ts Show resolved Hide resolved
packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/cx-api/README.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/cx-api/FEATURE_FLAGS.md Show resolved Hide resolved
packages/@aws-cdk/cx-api/FEATURE_FLAGS.md Outdated Show resolved Hide resolved
packages/@aws-cdk/cx-api/FEATURE_FLAGS.md Show resolved Hide resolved
@paulhcsun paulhcsun added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jan 30, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 30, 2024 23:43

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@mergify mergify bot dismissed stale reviews from GavinZZ and paulhcsun February 1, 2024 07:08

Pull request has been modified.

@go-to-k
Copy link
Contributor Author

go-to-k commented Feb 1, 2024

@GavinZZ @paulhcsun

Thanks for your review. All review comments were reflected. I also have a few comments.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 1, 2024
@paulhcsun
Copy link
Contributor

Looks good to me, thanks @go-to-k! I'll defer the approval to @GavinZZ.

Copy link
Contributor

@GavinZZ GavinZZ left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for contributing!

Copy link
Contributor

mergify bot commented Feb 2, 2024

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

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 2, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 460316d
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 144b1b9 into aws:main Feb 2, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Feb 2, 2024

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

SankyRed pushed a commit that referenced this pull request Feb 8, 2024
…e (under feature flag) (#28556)

[The documentation](https://github.com/aws/aws-cdk/blob/f4c1d1253ee34c2837a57a93faa47c9da97ef6d8/packages/aws-cdk-lib/aws-codepipeline/lib/pipeline.ts#L380-L381) mentions updating the default for CDK v2. Sounds like we should add it in with feature flag.

Closes #28247.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TheRealAmazonKendra pushed a commit that referenced this pull request Feb 9, 2024
…e (under feature flag) (#28556)

[The documentation](https://github.com/aws/aws-cdk/blob/f4c1d1253ee34c2837a57a93faa47c9da97ef6d8/packages/aws-cdk-lib/aws-codepipeline/lib/pipeline.ts#L380-L381) mentions updating the default for CDK v2. Sounds like we should add it in with feature flag.

Closes #28247.

----

*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
distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1 pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(codepipeline): feature flag update to crossAccountKeys
4 participants