-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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 pipeline type to V2 (under feature flag) #29096
feat(codepipeline): change default pipeline type to V2 (under feature flag) #29096
Conversation
There was a problem hiding this 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.
pipelineType
to V2 (under feature flag)
pipelineType
to V2 (under feature flag)✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
const app = new App({ | ||
postCliContext: { | ||
'@aws-cdk/aws-codepipeline:defaultPipelineTypeToV2': false, | ||
}, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is not specified, CodeBuild CI will error.
@aws-cdk-testing/framework-integ: CHANGED pipelines/test/integ.pipeline 1.467s
@aws-cdk-testing/framework-integ: Resources
@aws-cdk-testing/framework-integ: [~] AWS::CodePipeline::Pipeline Pipeline9850B417
@aws-cdk-testing/framework-integ: 笏披楳 [+] PipelineType
@aws-cdk-testing/framework-integ: 笏披楳 V2
@aws-cdk-testing/framework-integ: Snapshot Results:
@aws-cdk-testing/framework-integ: Tests: 41 failed, 801 total
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/cloudformation/integ.stacksets.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.lambda-pipeline.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-cfn-cross-region.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-cfn-with-action-role.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-cfn.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-build-batch.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-build-multiple-inputs-outputs.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-commit-build.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-commit-custom-event.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-commit-main.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-commit.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-deploy-ecs.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-code-deploy.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-ecr-source.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-ecs-deploy.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-elastic-beanstalk-deploy.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-events.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-jenkins.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-manual-approval.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-stepfunctions.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/integ.pipeline-with-replication.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-codepipeline-actions/test/s3/integ.source-bucket-events-cross-stack-same-env.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-events-targets/test/codepipeline/integ.pipeline-event-target.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-codebuild-logging.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-account-keys.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-region-replication-buckets.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-file-system-locations.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-vpc.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline-security.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline-with-artifact-bucket.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline-with-assets-single-upload.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline-with-assets.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline-with-stack-outputs-in-custom-step.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline-with-variables.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline-without-prepare.js
@aws-cdk-testing/framework-integ: Failed: /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.pipeline.js
@aws-cdk-testing/framework-integ: Error: Some tests failed!
@aws-cdk-testing/framework-integ: To re-run failed tests run: integ-runner --update-on-failed
@aws-cdk-testing/framework-integ: at main (/codebuild/output/src3346000757/src/github.com/aws/aws-cdk/packages/@aws-cdk/integ-runner/lib/cli.js:191:19)
@aws-cdk-testing/framework-integ: Error: integ-runner exited with error code 1
@aws-cdk-testing/framework-integ: Tests failed. Total time (2m5.3s) | integ-runner (1m49.2s) | /codebuild/output/src3346000757/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js (14.2s)
@aws-cdk-testing/framework-integ: !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused why we need to set the feature flag to false
in context. Wouldn't this check be false
when no context is provided and doesn't change the integ test behaviour?
const isDefaultV2 = FeatureFlags.of(this).isEnabled(cxapi.CODEPIPELINE_DEFAULT_PIPELINE_TYPE_TO_V2);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am confused too, but I actually got the error in the CodeBuild CI. I thought there was no difference in the local environment...
I'm willing to take time somewhere to investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As expected, it appears that if this change is not made, there will be a difference. However, the difference does not occur in my local environment (using the integ runner). I guess that perhaps the CodeBuild settings running in CI Workflow (or build.sh) are different.
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/cloudformation/integ.stacksets 2.077s
@aws-cdk-testing/framework-integ: Resources
@aws-cdk-testing/framework-integ: [~] AWS::CodePipeline::Pipeline PipelineC660917D
@aws-cdk-testing/framework-integ: 笏披楳 [+] PipelineType
@aws-cdk-testing/framework-integ: 笏披楳 V2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explain the feature flag in the CodePipeline README? I think that would be useful. Otherwise it looks good from my point of view. 👍
Thank you for the review. I added the doc for the feature flag in REAME. Could you please take a look at it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Left some questions and feedback.
@@ -549,7 +550,7 @@ export class Pipeline extends PipelineBase { | |||
disableInboundStageTransitions: Lazy.any({ produce: () => this.renderDisabledTransitions() }, { omitEmptyArray: true }), | |||
roleArn: this.role.roleArn, | |||
restartExecutionOnUpdate: props && props.restartExecutionOnUpdate, | |||
pipelineType: props.pipelineType, | |||
pipelineType: props.pipelineType ?? (isDefaultV2 ? PipelineType.V2 : undefined), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: seems redundant check since on line 536, you've already used null coalescing operator ??
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line 536 is the following:
this.pipelineType = props.pipelineType ?? (isDefaultV2 ? PipelineType.V2 : PipelineType.V1);
The difference between the line 536 and the code is whether it sets undefined or PipelineType.V1
when props.pipelineType
is undefined and the isDefaultV2
is false.
Without this code (not this.pipelineType
), if someone who did not originally specify props.pipelineType
upgrades to the version that includes this change, the PipelineType.V1
will be explicitly set to L1 if the feature flag is left off. This is by no means a breaking change, but it does mean that the version upgrade will cause the snapshot test to fail. In order to avoid changing the behavior with previous versions, it was intended that the template would be the same as the existing one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh thanks for the explanation.
@@ -51,8 +51,9 @@ class PipelineStack extends Stack { | |||
} | |||
|
|||
const app = new App({ | |||
context: { | |||
postCliContext: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not actually aware of the difference between context
vs postCliContext
. Can you explain why to use postCliContext
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because it didn't work with the actual context
. I would like to investigate this as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried putting it back in and some seem to succeed and others fail.
PS) Extracted tests where differences occurred in context
.
@aws-cdk/aws-appconfig-alpha: CHANGED integ.configuration 2.097s
@aws-cdk-testing/framework-integ: CHANGED aws-appconfig/test/integ.configuration 3.625s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/cloudformation/integ.stacksets 1.281s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.lambda-deployed-through-codepipeline.lit 2.179s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-ecr-source 0.986s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.cfn-template-from-repo.lit 3.579s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-alexa-deploy 3.408s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.lambda-pipeline 3.984s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-cfn-cross-region 3.705s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-cfn-with-action-role 3.668s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-code-build-batch 3.765s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-code-build-multiple-inputs-outputs 3.833s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-cfn 4.065s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-ecs-deploy 2.863s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-code-deploy-ecs 3.987s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-code-commit-build 4.418s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-events 2.853s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-jenkins 2.594s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/s3/integ.source-bucket-events-cross-stack-same-env 1.591s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-with-replication 1.803s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-stepfunctions 2.689s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-elastic-beanstalk-deploy 5.364s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-code-deploy 9.396s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-s3-deploy 9.515s
@aws-cdk-testing/framework-integ: CHANGED aws-codepipeline-actions/test/integ.pipeline-manual-approval 11.714s
@aws-cdk-testing/framework-integ: CHANGED aws-events-targets/test/codepipeline/integ.pipeline-event-target 2.799s
@aws-cdk-testing/framework-integ: CHANGED pipelines/test/integ.pipeline-with-assets-single-upload 1.59s
@aws-cdk-testing/framework-integ: CHANGED pipelines/test/integ.pipeline-with-assets 1.703s
@aws-cdk-testing/framework-integ: CHANGED pipelines/test/integ.pipeline-security 2.06s
@aws-cdk-testing/framework-integ: CHANGED pipelines/test/integ.pipeline 1.397s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your merge.
I did a little research and it seems that the following line does not cause a difference even in context
.
/// !cdk-integ VariablePipelineStack pragma:set-context:@aws-cdk/core:newStyleStackSynthesis=true
However, using DefaultStackSynthesizer
in the stack definition seems to cause a difference due to feature flags even if the above line is present.
const stack = new TestCdkStack(app, 'PipelineSecurityStack', {
synthesizer: new DefaultStackSynthesizer(),
});
I am not sure because I have not read the source code thoroughly...
Regarding "context vs postCliContext", it seems that the postCliContext
has priority over the context (and perhaps CLI contexts), environments, etc.
@GavinZZ Thanks for your review. I commented to your comments. Could you please check them? |
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
Reason for this change
We discussed about the default pipeline type, it was concluded that the new type (V2) should be the default.
#28538 (comment)
https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-codepipeline/lib/pipeline.ts#L492
Description of changes
Change the default value using a feature flag so as not to affect existing processing.
Description of how you validated changes
Bot unit and integ tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license