fix(codepipeline-actions): cross stack reference causes stack cycle in sources that use CloudWatch Events #20149

skinny85 · 2022-04-29T18:04:08Z

When using a newly-created, CDK-managed resource, such as an S3 Bucket or a CodeCommit Repository,
as the source of a CodePipeline, when it's in a different Stack than the pipeline
(but in the same environment as it),
and we use CloudWatch Events to trigger the pipeline from the source,
that would result in a cycle:

Because the Event Rule is created in the source Stack, that Stack needs the name of the pipeline to trigger from the Rule, and also the Role to use for the trigger, which is created in the target (in this case, the pipeline) Stack. So, the source Stack depends on the pipeline Stack.
The pipeline Stack needs the name of the source resource. So, the pipeline Stack depends on the source Stack.

The only way to break this cycle is to move the Event Rule to the target Stack.
This PR adds an API to the Events module to make it possible for event-creating constructs to make that choice,
and uses that capability in the CodePipeline CodeCommitSourceAction and S3SourceAction.

Fixes #3087
Fixes #8042
Fixes #10896

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

…cross-environment.

gitpod-io · 2022-04-29T18:04:15Z

skinny85 · 2022-04-29T18:06:40Z

@rix0rrr would be curious to get your opinion on whether you like this approach!

skinny85 · 2022-04-29T18:07:25Z

packages/@aws-cdk/aws-events/lib/on-event-options.ts

+   *
+   * @default - none (the main scope will be used, even for cross-stack Events)
+   */
+  readonly scopeIfCrossStack?: Construct;


This name definitely sucks, so I'm very open to suggestions for a better one.

Maybe just crossStackScope?

OK, I'll update to crossStackScope.

skinny85 · 2022-04-29T22:24:49Z

Note that, before this PR is merged, we should make a similar change that we have now in CodeCommitSourceAction also in S3SourceAction.

TheRealAmazonKendra

So, I think I like this but I'd definitely want to see more until testing and some integration tests.

TheRealAmazonKendra · 2022-08-02T20:08:33Z

packages/@aws-cdk/aws-events/lib/on-event-options.ts

+   *
+   * @default - none (the main scope will be used, even for cross-stack Events)
+   */
+  readonly scopeIfCrossStack?: Construct;


Maybe just crossStackScope?

TheRealAmazonKendra · 2022-08-02T20:15:59Z

packages/@aws-cdk/aws-events/lib/rule.ts

@@ -119,7 +88,7 @@ export class Rule extends Resource implements IRule {
  private readonly _xEnvTargetsAdded = new Set<string>();

  constructor(scope: Construct, id: string, props: RuleProps = { }) {
-    super(scope, id, {
+    super(determineRuleScope(scope, props), id, {


Tests for this?

This is verified by every single test that uses any Rule, of which there are hundreds, and also the new "allows using a new Repository from another Stack as a source of the Pipeline, with Events" test that was added in this PR.

Fair. I can let that go. I suppose I'm reasonable every once in a while about my testing demands 🙂

Can we get an integration test on this, though? It seems like exactly the sort of thing that we'd want integ tests for.

Pull request has been modified.

TheRealAmazonKendra

Just putting this in changes requested so I see when an update has been made. You mentioned that this sam change should be made for S3SourceAction. Is that a change you'd like to make or should I go ahead and take that on?

skinny85 · 2022-08-04T04:53:40Z

Just putting this in changes requested so I see when an update has been made. You mentioned that this sam change should be made for S3SourceAction. Is that a change you'd like to make or should I go ahead and take that on?

I'll do it. I'll combine it with the integration test.

skinny85 · 2022-08-07T05:48:36Z

Just putting this in changes requested so I see when an update has been made. You mentioned that this sam change should be made for S3SourceAction. Is that a change you'd like to make or should I go ahead and take that on?

I'll do it. I'll combine it with the integration test.

Done in the latest revision. @TheRealAmazonKendra would appreciate another look!

Pull request has been modified.

TheRealAmazonKendra

Just one more issue in here. Looks like this isn't using the new IntegTest construct to run the integration test. Can you please make that update? Besides that, this looks good to go!

skinny85 · 2022-08-09T05:01:30Z

Just one more issue in here. Looks like this isn't using the new IntegTest construct to run the integration test. Can you please make that update? Besides that, this looks good to go!

Done!

Pull request has been modified.

TheRealAmazonKendra

Looks great!

mergify · 2022-08-09T05:57:32Z

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).

skinny85 · 2022-08-09T06:18:59Z

Is main broken right now...?

@aws-cdk/integ-runner: PASS test/runner/snapshot-test-runner.test.js
@aws-cdk/integ-runner: PASS test/runner/integ-test-runner.test.js
@aws-cdk/integ-runner: FAIL test/workers/integ-worker.test.js
@aws-cdk/integ-runner:   â—� test runner â€º has snapshot
@aws-cdk/integ-runner:     expect(received).toEqual(expected) // deep equality
@aws-cdk/integ-runner:     - Expected  - 1
@aws-cdk/integ-runner:     + Received  + 6
@aws-cdk/integ-runner:     - Array []
@aws-cdk/integ-runner:     + Array [
@aws-cdk/integ-runner:     +   Object {
@aws-cdk/integ-runner:     +     "discoveryRoot": "test/test-data",
@aws-cdk/integ-runner:     +     "fileName": "test/test-data/xxxxx.test-with-snapshot.js",
@aws-cdk/integ-runner:     +   },
@aws-cdk/integ-runner:     + ]
@aws-cdk/integ-runner:       150 |     ]));
@aws-cdk/integ-runner:       151 |
@aws-cdk/integ-runner:     > 152 |     expect(results).toEqual([]);
@aws-cdk/integ-runner:           |                     ^
@aws-cdk/integ-runner:       153 |   });
@aws-cdk/integ-runner:       154 |
@aws-cdk/integ-runner:       155 |   test('deploy failed', () => {
@aws-cdk/integ-runner:       at Object.<anonymous> (test/workers/integ-worker.test.ts:152:21)
@aws-cdk/integ-runner: PASS test/workers/snapshot-worker.test.js
@aws-cdk/integ-runner: =============================== Coverage summary ===============================
@aws-cdk/integ-runner: Statements   : 81.1% ( 571/704 )
@aws-cdk/integ-runner: Branches     : 71.94% ( 377/524 )
@aws-cdk/integ-runner: Functions    : 81.1% ( 103/127 )
@aws-cdk/integ-runner: Lines        : 81.21% ( 549/676 )
@aws-cdk/integ-runner: ================================================================================
@aws-cdk/integ-runner: Test Suites: 1 failed, 7 passed, 8 total
@aws-cdk/integ-runner: Tests:       1 failed, 63 passed, 64 total
@aws-cdk/integ-runner: Snapshots:   0 total
@aws-cdk/integ-runner: Time:        4.34 s
@aws-cdk/integ-runner: Ran all test suites.
@aws-cdk/integ-runner: Error: /codebuild/output/src162968263/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js exited with error code 1
@aws-cdk/integ-runner: Tests failed. Total time (5.3s) | /codebuild/output/src162968263/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js (5.3s)

TheRealAmazonKendra · 2022-08-09T21:41:54Z

@Mergifyio update

mergify · 2022-08-09T21:42:14Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra · 2022-08-09T21:42:33Z

Is main broken right now...?

@aws-cdk/integ-runner: PASS test/runner/snapshot-test-runner.test.js
@aws-cdk/integ-runner: PASS test/runner/integ-test-runner.test.js
@aws-cdk/integ-runner: FAIL test/workers/integ-worker.test.js
@aws-cdk/integ-runner:   â—� test runner â€º has snapshot
@aws-cdk/integ-runner:     expect(received).toEqual(expected) // deep equality
@aws-cdk/integ-runner:     - Expected  - 1
@aws-cdk/integ-runner:     + Received  + 6
@aws-cdk/integ-runner:     - Array []
@aws-cdk/integ-runner:     + Array [
@aws-cdk/integ-runner:     +   Object {
@aws-cdk/integ-runner:     +     "discoveryRoot": "test/test-data",
@aws-cdk/integ-runner:     +     "fileName": "test/test-data/xxxxx.test-with-snapshot.js",
@aws-cdk/integ-runner:     +   },
@aws-cdk/integ-runner:     + ]
@aws-cdk/integ-runner:       150 |     ]));
@aws-cdk/integ-runner:       151 |
@aws-cdk/integ-runner:     > 152 |     expect(results).toEqual([]);
@aws-cdk/integ-runner:           |                     ^
@aws-cdk/integ-runner:       153 |   });
@aws-cdk/integ-runner:       154 |
@aws-cdk/integ-runner:       155 |   test('deploy failed', () => {
@aws-cdk/integ-runner:       at Object.<anonymous> (test/workers/integ-worker.test.ts:152:21)
@aws-cdk/integ-runner: PASS test/workers/snapshot-worker.test.js
@aws-cdk/integ-runner: =============================== Coverage summary ===============================
@aws-cdk/integ-runner: Statements   : 81.1% ( 571/704 )
@aws-cdk/integ-runner: Branches     : 71.94% ( 377/524 )
@aws-cdk/integ-runner: Functions    : 81.1% ( 103/127 )
@aws-cdk/integ-runner: Lines        : 81.21% ( 549/676 )
@aws-cdk/integ-runner: ================================================================================
@aws-cdk/integ-runner: Test Suites: 1 failed, 7 passed, 8 total
@aws-cdk/integ-runner: Tests:       1 failed, 63 passed, 64 total
@aws-cdk/integ-runner: Snapshots:   0 total
@aws-cdk/integ-runner: Time:        4.34 s
@aws-cdk/integ-runner: Ran all test suites.
@aws-cdk/integ-runner: Error: /codebuild/output/src162968263/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js exited with error code 1
@aws-cdk/integ-runner: Tests failed. Total time (5.3s) | /codebuild/output/src162968263/src/github.com/aws/aws-cdk/node_modules/jest/bin/jest.js (5.3s)

Wut? Gotta be a transient issue. Let's see what the retry does.

mergify · 2022-08-09T22:18:35Z

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 · 2022-08-09T22:54:41Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 87b5e67
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2022-08-09T22:55:08Z

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).

…n sources that use CloudWatch Events (aws#20149) When using a newly-created, CDK-managed resource, such as an S3 Bucket or a CodeCommit Repository, as the source of a CodePipeline, when it's in a different Stack than the pipeline (but in the same environment as it), and we use CloudWatch Events to trigger the pipeline from the source, that would result in a cycle: 1. Because the Event Rule is created in the source Stack, that Stack needs the name of the pipeline to trigger from the Rule, and also the Role to use for the trigger, which is created in the target (in this case, the pipeline) Stack. So, the source Stack depends on the pipeline Stack. 2. The pipeline Stack needs the name of the source resource. So, the pipeline Stack depends on the source Stack. The only way to break this cycle is to move the Event Rule to the target Stack. This PR adds an API to the Events module to make it possible for event-creating constructs to make that choice, and uses that capability in the CodePipeline `CodeCommitSourceAction` and `S3SourceAction`. Fixes aws#3087 Fixes aws#8042 Fixes aws#10896 ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

skinny85 added 3 commits April 29, 2022 10:34

Hacky first version.

412b944

First unit test - unfortunately, breaks a different one!

53a5a57

Make the scope resolution be dependent on whether the scopes are not …

e71a56f

…cross-environment.

skinny85 requested a review from rix0rrr April 29, 2022 18:04

mergify bot added the contribution/core This is a PR that came from AWS. label Apr 29, 2022

github-actions bot added bug This issue is a bug. effort/large Large work item – several weeks of effort p1 labels Apr 29, 2022

aws-cdk-automation requested a review from a team April 29, 2022 18:04

skinny85 commented Apr 29, 2022

View reviewed changes

TheRealAmazonKendra changed the base branch from v1-main to main July 13, 2022 00:07

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

Merge branch 'main' into codepipeline-cycle-sources-events-10896

efee8d9

TheRealAmazonKendra removed the contribution/core This is a PR that came from AWS. label Aug 3, 2022

Rename scopeIfCrossStack to crossStackScope.

8b463d5

skinny85 requested a review from TheRealAmazonKendra August 3, 2022 05:11

TheRealAmazonKendra previously requested changes Aug 3, 2022

View reviewed changes

TheRealAmazonKendra changed the title ~~fix(codepipeline): break cycle for cross-Stack sources using Events~~ fix(codepipeline-actions): cross stack reference causes cyclic reference error in CodeCommitSourceAction Aug 3, 2022

Add integration test.

a014fcc

skinny85 requested a review from TheRealAmazonKendra August 7, 2022 05:48

skinny85 changed the title ~~fix(codepipeline-actions): cross stack reference causes cyclic reference error in CodeCommitSourceAction~~ fix(codepipeline-actions): cross stack reference causes cyclic reference error in sources that use CloudWatch Events Aug 7, 2022

skinny85 changed the title ~~fix(codepipeline-actions): cross stack reference causes cyclic reference error in sources that use CloudWatch Events~~ fix(codepipeline-actions): cross stack reference causes stack cycle in sources that use CloudWatch Events Aug 7, 2022

TheRealAmazonKendra previously requested changes Aug 8, 2022

View reviewed changes

Use the IntegTest construct.

9e261d7

skinny85 requested a review from TheRealAmazonKendra August 9, 2022 05:01

TheRealAmazonKendra approved these changes Aug 9, 2022

View reviewed changes

Merge branch 'main' into codepipeline-cycle-sources-events-10896

0eb6f87

Merge branch 'main' into codepipeline-cycle-sources-events-10896

a314f9d

Merge branch 'main' into codepipeline-cycle-sources-events-10896

87b5e67

mergify bot merged commit adf4022 into aws:main Aug 9, 2022

skinny85 deleted the codepipeline-cycle-sources-events-10896 branch August 10, 2022 05:06

misterjoshua mentioned this pull request Aug 16, 2022

feat(cli): cdk watch --concurrency #21598

Merged

4 tasks

skinny85 mentioned this pull request Jan 29, 2023

Create repo in stack A and ref it to codepipeline in stack B throws 'cyclic reference' #5727

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(codepipeline-actions): cross stack reference causes stack cycle in sources that use CloudWatch Events #20149

fix(codepipeline-actions): cross stack reference causes stack cycle in sources that use CloudWatch Events #20149

skinny85 commented Apr 29, 2022 •

edited

Loading

gitpod-io bot commented Apr 29, 2022

skinny85 commented Apr 29, 2022

skinny85 Apr 29, 2022

TheRealAmazonKendra Aug 2, 2022

skinny85 Aug 3, 2022

skinny85 commented Apr 29, 2022

TheRealAmazonKendra left a comment

TheRealAmazonKendra Aug 2, 2022

TheRealAmazonKendra Aug 2, 2022

skinny85 Aug 3, 2022

TheRealAmazonKendra Aug 3, 2022 •

edited

Loading

TheRealAmazonKendra Aug 3, 2022

skinny85 Aug 3, 2022

TheRealAmazonKendra left a comment

skinny85 commented Aug 4, 2022 •

edited

Loading

skinny85 commented Aug 7, 2022

TheRealAmazonKendra left a comment

skinny85 commented Aug 9, 2022

TheRealAmazonKendra left a comment

mergify bot commented Aug 9, 2022

skinny85 commented Aug 9, 2022

TheRealAmazonKendra commented Aug 9, 2022

mergify bot commented Aug 9, 2022

TheRealAmazonKendra commented Aug 9, 2022

mergify bot commented Aug 9, 2022

aws-cdk-automation commented Aug 9, 2022

mergify bot commented Aug 9, 2022

fix(codepipeline-actions): cross stack reference causes stack cycle in sources that use CloudWatch Events #20149

fix(codepipeline-actions): cross stack reference causes stack cycle in sources that use CloudWatch Events #20149

Conversation

skinny85 commented Apr 29, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Apr 29, 2022

skinny85 commented Apr 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 commented Apr 29, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealAmazonKendra Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

skinny85 commented Aug 4, 2022 • edited Loading

skinny85 commented Aug 7, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

skinny85 commented Aug 9, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

mergify bot commented Aug 9, 2022

skinny85 commented Aug 9, 2022

TheRealAmazonKendra commented Aug 9, 2022

mergify bot commented Aug 9, 2022

✅ Branch has been successfully updated

TheRealAmazonKendra commented Aug 9, 2022

mergify bot commented Aug 9, 2022

aws-cdk-automation commented Aug 9, 2022

AWS CodeBuild CI Report

mergify bot commented Aug 9, 2022

skinny85 commented Apr 29, 2022 •

edited

Loading

TheRealAmazonKendra Aug 3, 2022 •

edited

Loading

skinny85 commented Aug 4, 2022 •

edited

Loading