-
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(pipelines): expose crossRegionReplicationBuckets #28447
feat(pipelines): expose crossRegionReplicationBuckets #28447
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.
I don't see any integration tests for the |
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 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/lib/codepipeline/codepipeline.ts
Outdated
Show resolved
Hide resolved
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? |
added crr buckets integ test
…kets' into add-CRR-buckets-integ-test
@jose-clickup added an IT for us. I'm trying to figure out how to get a snapshot generated. |
feat: add crr buckets integ test
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
Add crr buckets integ test
fix: modified README to fix the build
packages/aws-cdk-lib/pipelines/lib/codepipeline/codepipeline.ts
Outdated
Show resolved
Hide resolved
const pipeline = new pipelines.CodePipeline(this, 'Pipeline', { | ||
synth: new pipelines.ShellStep('Synth', { | ||
input: pipelines.CodePipelineSource.gitHub( | ||
'jose-clickup/cdk-pipelines-demo', |
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.
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.
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.
@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?
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.
@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
...amework-integ/test/pipelines/test/integ.newpipeline-with-cross-region-replication-buckets.ts
Show resolved
Hide resolved
721bbd7
to
7a8be3c
Compare
Relates to AWS Support case 14415967131 in our usCdkPipelines account. |
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. |
Thank you for the response. We just passed 930 of the 1k hard limit on
buckets in our cdkPipelines account, so this is urgent for us.
MiniLockID: uX4VrN5FvyFxFCxgTksGxJqvKa16iBhqseYxxA1UkZVJw
GPG: 773A 6BDD 71CE 0AB8 0F5A 1176 8679 A114 FB1A 69BD
…On Fri, Jan 5, 2024 at 6:23 PM Kaizen Conroy ***@***.***> wrote:
Thank you for the additional information @ahammond
<https://github.com/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.
—
Reply to this email directly, view it on GitHub
<#28447 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADM2RFXH65OH52RGVHZA73YNCYSDAVCNFSM6AAAAABA5TYW6GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZZGUYDEMRRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
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.
@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!
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). |
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). |
Closes #28446.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license