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(pipelines): expose crossRegionReplicationBuckets #28447

Merged
merged 29 commits into from
Jan 8, 2024

Conversation

ahammond
Copy link
Contributor

Closes #28446.


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

@github-actions github-actions bot added feature-request A feature should be added or improved. p2 labels Dec 21, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team December 21, 2023 00:35
@github-actions github-actions bot added the repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK label Dec 21, 2023
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.

@ahammond
Copy link
Contributor Author

I don't see any integration tests for the pipelines module and that is beyond the scope of what I can justify working on at work.

Copy link
Contributor

@kaizencc kaizencc 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 your contribution @ahammond. Your code looks good, and I appreciate the note in the readme as well. Can you add an integ test to this module? That's the only thing that's missing, and I think important here to make sure you can use your new property correctly. It will also help as just an additional example for how to do what you're describing

packages/aws-cdk-lib/pipelines/README.md Outdated Show resolved Hide resolved
packages/aws-cdk-lib/pipelines/README.md Outdated Show resolved Hide resolved
@kaizencc
Copy link
Contributor

I don't see any integration tests for the pipelines module and that is beyond the scope of what I can justify working on at work.

I just saw this @ahammond . Unfortunately I think its important to have it here. This PR will likely fester unless some other good samaritan is willing to add the integ test. However, if you're willing, we have the integ tests living in this folder. If not, maybe you can see if someone in cdk.dev #contributing channel is willing to move this over the finish line?

@ahammond
Copy link
Contributor Author

ahammond commented Jan 2, 2024

@jose-clickup added an IT for us. I'm trying to figure out how to get a snapshot generated.

@aws-cdk-automation aws-cdk-automation dismissed their stale review January 2, 2024 23:17

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

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 4, 2024
const pipeline = new pipelines.CodePipeline(this, 'Pipeline', {
synth: new pipelines.ShellStep('Synth', {
input: pipelines.CodePipelineSource.gitHub(
'jose-clickup/cdk-pipelines-demo',
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @jose-clickup thanks for the clarifications. I am not comfortable with using a source repository that is not owned by us in this integ test. Can you use the repo in aws-samples instead? Or are there some permissioning issues beneath the surface here as well. Sorry, I've forgotten how much of a pain pipelines integ tests are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kaizencc there are numerous examples of this pattern throughout the code.

❯ rg cdk-pipelines-demo

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline.js.snapshot/PipelineStackPipeline9DB740AF.dot
10:"Source.rix0rrr/cdk-pipelines-demo" -> "Build.Synth";
189:"Source.rix0rrr/cdk-pipelines-demo";
190:"BEGIN Source" -> "Source.rix0rrr/cdk-pipelines-demo";
191:"Source.rix0rrr/cdk-pipelines-demo" -> "END Source";

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline.js.snapshot/tree.json
343:                                "name": "rix0rrr_cdk-pipelines-demo",
357:                                  "Repo": "cdk-pipelines-demo",
1953:                      "rix0rrr_cdk-pipelines-demo": {
1954:                        "id": "rix0rrr_cdk-pipelines-demo",
1955:                        "path": "PipelineStack/Pipeline/Pipeline/Source/rix0rrr_cdk-pipelines-demo",
1959:                            "path": "PipelineStack/Pipeline/Pipeline/Source/rix0rrr_cdk-pipelines-demo/WebhookResource",
1973:                                "targetAction": "rix0rrr_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline.js.snapshot/manifest.json
115:        "/PipelineStack/Pipeline/Pipeline/Source/rix0rrr_cdk-pipelines-demo/WebhookResource": [

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline.js.snapshot/PipelineStack.template.json
253:         "Repo": "cdk-pipelines-demo",
258:        "Name": "rix0rrr_cdk-pipelines-demo",
1864:    "TargetAction": "rix0rrr_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline.ts
14:        input: pipelines.CodePipelineSource.gitHub('rix0rrr/cdk-pipelines-demo', 'main'),

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-account-keys.ts
16:        input: pipelines.CodePipelineSource.gitHub('tkglaser/cdk-pipelines-demo', 'main'),

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-region-replication-buckets.ts
51:          'jose-clickup/cdk-pipelines-demo',

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-region-replication-buckets.js.snapshot/PipelineStack.template.json
161:         "Repo": "cdk-pipelines-demo",
166:        "Name": "jose-clickup_cdk-pipelines-demo",
468:    "TargetAction": "jose-clickup_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-region-replication-buckets.js.snapshot/PipelineStackPipeline9DB740AF.dot
10:"Source.jose-clickup/cdk-pipelines-demo" -> "Build.Synth";
52:"Source.jose-clickup/cdk-pipelines-demo";
53:"BEGIN Source" -> "Source.jose-clickup/cdk-pipelines-demo";
54:"Source.jose-clickup/cdk-pipelines-demo" -> "END Source";

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-region-replication-buckets.js.snapshot/manifest.json
239:        "/PipelineStack/Pipeline/Pipeline/Source/jose-clickup_cdk-pipelines-demo/WebhookResource": [

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-region-replication-buckets.js.snapshot/tree.json
807:                                "name": "jose-clickup_cdk-pipelines-demo",
821:                                  "Repo": "cdk-pipelines-demo",
1112:                      "jose-clickup_cdk-pipelines-demo": {
1113:                        "id": "jose-clickup_cdk-pipelines-demo",
1114:                        "path": "PipelineStack/Pipeline/Pipeline/Source/jose-clickup_cdk-pipelines-demo",
1118:                            "path": "PipelineStack/Pipeline/Pipeline/Source/jose-clickup_cdk-pipelines-demo/WebhookResource",
1133:                                "targetAction": "jose-clickup_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-account-keys.js.snapshot/manifest.json
127:        "/PipelineStack/Pipeline/Pipeline/Source/tkglaser_cdk-pipelines-demo/WebhookResource": [

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-account-keys.js.snapshot/PipelineStack.template.json
358:         "Repo": "cdk-pipelines-demo",
363:        "Name": "tkglaser_cdk-pipelines-demo",
1978:    "TargetAction": "tkglaser_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-account-keys.js.snapshot/tree.json
480:                                "name": "tkglaser_cdk-pipelines-demo",
494:                                  "Repo": "cdk-pipelines-demo",
2099:                      "tkglaser_cdk-pipelines-demo": {
2100:                        "id": "tkglaser_cdk-pipelines-demo",
2101:                        "path": "PipelineStack/Pipeline/Pipeline/Source/tkglaser_cdk-pipelines-demo",
2105:                            "path": "PipelineStack/Pipeline/Pipeline/Source/tkglaser_cdk-pipelines-demo/WebhookResource",
2119:                                "targetAction": "tkglaser_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-cross-account-keys.js.snapshot/PipelineStackPipeline9DB740AF.dot
10:"Source.tkglaser/cdk-pipelines-demo" -> "Build.Synth";
189:"Source.tkglaser/cdk-pipelines-demo";
190:"BEGIN Source" -> "Source.tkglaser/cdk-pipelines-demo";
191:"Source.tkglaser/cdk-pipelines-demo" -> "END Source";

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-codebuild-logging.js.snapshot/PipelineStackPipeline9DB740AF.dot
10:"Source.colifran/cdk-pipelines-demo" -> "Build.Synth";
189:"Source.colifran/cdk-pipelines-demo";
190:"BEGIN Source" -> "Source.colifran/cdk-pipelines-demo";
191:"Source.colifran/cdk-pipelines-demo" -> "END Source";

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-codebuild-logging.js.snapshot/tree.json
359:                                "name": "colifran_cdk-pipelines-demo",
373:                                  "Repo": "cdk-pipelines-demo",
1969:                      "colifran_cdk-pipelines-demo": {
1970:                        "id": "colifran_cdk-pipelines-demo",
1971:                        "path": "PipelineStack/Pipeline/Pipeline/Source/colifran_cdk-pipelines-demo",
1975:                            "path": "PipelineStack/Pipeline/Pipeline/Source/colifran_cdk-pipelines-demo/WebhookResource",
1989:                                "targetAction": "colifran_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-codebuild-logging.js.snapshot/manifest.json
115:        "/PipelineStack/Pipeline/Pipeline/Source/colifran_cdk-pipelines-demo/WebhookResource": [

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-codebuild-logging.js.snapshot/PipelineStack.template.json
253:         "Repo": "cdk-pipelines-demo",
258:        "Name": "colifran_cdk-pipelines-demo",
1864:    "TargetAction": "colifran_cdk-pipelines-demo",

packages/@aws-cdk-testing/framework-integ/test/pipelines/test/integ.newpipeline-with-codebuild-logging.ts
20:        input: pipelines.CodePipelineSource.gitHub('colifran/cdk-pipelines-demo', 'main'),

This is pretty urgent for us. Can we get this merged and then, if you guys feel strongly about it, can you please do a cleanup?

Choose a reason for hiding this comment

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

@kaizencc, there are permissions issues because we're not part of the aws-samples org (despite it being public). This makes sense as from CodePipeline you are only able to select a repo that you own (listed using your github-token) from the GitHub connection

@ahammond ahammond force-pushed the expose-crossRegionReplicationBuckets branch from 721bbd7 to 7a8be3c Compare January 4, 2024 22:37
@ahammond
Copy link
Contributor Author

ahammond commented Jan 5, 2024

Relates to AWS Support case 14415967131 in our usCdkPipelines account.

@kaizencc
Copy link
Contributor

kaizencc commented Jan 6, 2024

Thank you for the additional information @ahammond and I understand the urgency. I'm not comfortable with merging this PR as is because I'm not familiar with this pattern - the fact that it is done elsewhere in CDK is promising but doesn't necessarily mean it's right. I have called in other people who are more familiar who can weigh in Monday with either an approval or a different path forward. I hope that is acceptable and sorry for the delay.

@ahammond
Copy link
Contributor Author

ahammond commented Jan 6, 2024 via email

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

@jose-clickup @ahammond Approving this PR because the security risk cannot currently be exploited as we do not automatically run our integ tests. We do plan to in the future, which means that we have some future work to take the other tests with this situation and replace them with a parameter and then --dry-run them.

This means that the tests will only succeed when manually run with an environment variable added, but that will be the best case scenario for these tests going forward.

Either way, I appreciate the urgency you have to get this PR in so I'm approving and doing the other work at some later time. Will make sure that this PR gets in the next CDK release. Cheers!

Copy link
Contributor

mergify bot commented Jan 8, 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-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 8, 2024
Copy link
Contributor

mergify bot commented Jan 8, 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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 6f13109
  • 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 d184ac2 into aws:main Jan 8, 2024
9 checks passed
Copy link
Contributor

mergify bot commented Jan 8, 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request A feature should be added or improved. p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pipeline: should allow crossRegionReplicationBuckets passthrough to underlying CodePipeline construct
4 participants