feat(core): CfnResource depends on methods #20419

jusdino · 2022-05-19T16:40:42Z

Add some new methods to allow a minimal interface for viewing and editing resource-to-resource dependencies that mirrors the behavior of CfnResource.addDependsOn().

Related to #20418 - details for justification are there

All Submissions:

Have you followed the guidelines in our Contributing guide?
This PR adds new unconventional dependencies following the process described here
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)?

Adding new Unconventional Dependencies:

New Features

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

gitpod-io · 2022-05-19T16:40:48Z

comcalvi

I like the additions this makes. However, if the intended use case is for users to write

foo = stack.tryFindChild('Foo');
if (foo.obtainDependsOn().contains(bar)) {
  foo.removeDependsOn(bar);
fi
foo.addDependsOn(barReplacement);

we should also add a replaceDependsOn(bar, barReplacement) that does exactly what the above snippet does.

packages/@aws-cdk/core/test/cfn-resource.test.ts

packages/@aws-cdk/core/lib/cfn-resource.ts

packages/@aws-cdk/core/lib/deps.ts

comcalvi · 2022-06-15T22:47:24Z

packages/@aws-cdk/core/lib/deps.ts

+function resourceInCommonStackFor(element: CfnResource | Stack, commonStack: Stack): CfnResource {
+  const resource = Stack.isStack(element) ? element.nestedStackResource : element;
+  if (!resource) {
+    throw new Error('assertion failure'); // see "assertion" above


this is not a very descriptive error message.

packages/@aws-cdk/core/lib/stack.ts

Pull request has been modified.

packages/@aws-cdk/core/lib/deps.ts

packages/@aws-cdk/core/lib/cfn-resource.ts

packages/@aws-cdk/core/lib/stack.ts

comcalvi · 2022-07-14T17:25:52Z

packages/@aws-cdk/core/lib/stack.ts

+    let index;
+    let filteredReasons = filterReasons(dep, reasonFilter);
+    if (filteredReasons.length > 1) {
+      throw new Error(`Reason for dependency removal is too ambiguous: ${reasonFilter}`);


Suggested change

throw new Error(`Reason for dependency removal is too ambiguous: ${reasonFilter}`);

throw new Error(`There cannot be more than one reason for dependency removal, found: ${reasonFilter}`);

Took your message but swapped /reasonFilter/filteredReasons/ since I think the latter makes more sense with your message.

packages/@aws-cdk/core/test/cfn-resource.test.ts

packages/@aws-cdk/core/lib/stack.ts

Pull request has been modified.

jusdino · 2022-07-26T15:53:06Z

@comcalvi, based on what I learned in #20281, I think we'll eventually have to reopen this pull request from a personal fork of aws-cdk instead of this organization-owned fork, since your mergify bot cannot push changes back to this fork branch. If you're close to satisfied with this PR, let me know and I'll do that. I didn't want to do that before we were nearly there, since I didn't want to break up our workflow while we still had revisions to work out.

TheRealAmazonKendra

These test failures have to do with a bug we had when you last merged from main. Please rebase so we can see if the build is succeeding before we can provide another round of reviews.

Pull request has been modified.

TheRealAmazonKendra · 2022-08-03T03:04:38Z

@jusdino Go into the aws-cdk-lib package and run yarn pkglint. Commit the change to the README file for aws-cdk-lib.

TheRealAmazonKendra

@comcalvi is already providing great feedback but I just want to add that I think this needs more testing. I'd like to see both more unit test coverage for these changes and integration tests as well. If you have any questions about writing integration tests, see our integration test guide.

packages/@aws-cdk/core/lib/stack.ts

Pull request has been modified.

jusdino · 2022-08-08T23:00:38Z

Ok, I detected the Names.uniqueId bug in an existing unit test and added a two-part integration test. I can look into adding some more unit tests for further coverage next.

packages/@aws-cdk/core/lib/stack.ts

packages/@aws-cdk/aws-cloudformation/test/integ.core-deps.ts

packages/@aws-cdk/core/lib/cfn-resource.ts

packages/@aws-cdk/core/lib/deps.ts

packages/@aws-cdk/core/lib/stack.ts

jusdino · 2022-11-28T20:48:08Z

@comcalvi ?

comcalvi · 2022-11-29T00:04:16Z

I don't see any breaking changes here - what are you referring to? The closest thing I see is the addDependsOn -> addDependency deprecation, which I did specifically as a change requested by Amazon. I'm happy to roll that back, if that is ultimately what you want.

I missed that the method whose signature changed was internal, that's my mistake.

Traversing the construct tree to remove dependencies is exactly what this enables. With the existing code, there is no mechanism to do that. I'm not sure what your point is here. Can you help me understand?

Sorry I wasn't more clear; I do not understand this mechanism. The main thing I'm missing the usage of filterReasons(). What does this function do? Why do we need to filter any reasons at all? I see filterReasons() used in a few spots, and I don't see what this is doing.

Ok, I'm trying to boil down where my approach differs from what you're suggesting, since I don't think it's far from what I did. Aside from adding _removeAssemblyDependency() and removeDependency() and adding the helper methods to pull out the common code, I also:

The method additions and name changes are good, this comment was about the new mechanism. A comment block detailing this mechanism would be helpful. I'm not understanding what precise capability this is adding.

jusdino · 2022-11-29T23:53:52Z

Sorry I wasn't more clear; I do not understand this mechanism. The main thing I'm missing the usage of filterReasons(). What does this function do? Why do we need to filter any reasons at all? I see filterReasons() used in a few spots, and I don't see what this is doing.

Ah ok. So the filterReasons function is the core of what I'm explaining here:
#20419 (comment)

Would adding a comment block with something along those lines be helpful?

comcalvi · 2022-11-30T16:29:52Z

the method will resolve it to a stack-level dependency and, rather than remove the dependency, it will find the matching source and target in the StackDependencyReason, remove that reason, but leave the stack dependency in place, since it will still have the source: ResourceB, target: ResourceD dependency reason. If the user code also then does ResourceB.removeDependsOn(ResourceD), the method will again resolve to the stack level, remove the corresponding StackDependencyReason then, seeing that there is no longer a reason for the dependency, remove the Stack1 on Stack2 dependency as well.

I don't see the purpose of filtering reasons in this process though. From this, I understand that we need to track resource dependencies across stacks. I think today we don't do this, and instead only track the stack itself as a dependency; this is a problem. As you explain, if we only track the stack as a dependency and a user removes one dependency but not the other, we can't determine that the stack dependency should actually be left in place; therefore, we need a mechanism to track the resources themselves as dependencies and not just the stack.

With that in mind, I would expect the fundamental change to add the construct path to the dependency list, eg Stack2/Resource (where today, it's just Stack2). To achieve that, you're tracking source and target for each dependency. That makes sense. What doesn't make sense to me is why we need to filter the dependency reasons. What problem does filtering dependencies sovle?

jusdino · 2022-12-05T18:25:31Z

What doesn't make sense to me is why we need to filter the dependency reasons. What problem does filtering dependencies sovle?

Sorry for the slow reply - I've been sick and wanted to think this through. My short answer is, looking closer, the filterReasons may be a bit overkill.

What we really need is to be able to match a StackDependencyReason reliably when removing resource-to-resource dependencies that ended up resolving as stack-to-stack dependencies. The filterReasons function is doing this for us but it may be a bit more complex than necessary specifically because I was trying to be a bit defensive in my approach. I was specifically thinking about the situation where, somehow, a source or target of null or undefined was added to the stackDependency.reasons. I don't think the public interface allows such a thing to happen, but if it did, I wanted a smarter approach to pulling out a match than specifically looking for source == reason.source && target == reason.target. Otherwise, that reason would never match and so would prevent the StackDependency from ever being completely removed.

It sounds like your preference would be to simplify. I could simplify/remove the filterReasons function if you would rather leave that null/undefined unaddressed, which I think is probably fine since it shouldn't happen, anyway.

comcalvi · 2022-12-05T19:42:14Z

Thanks for the explanation, this makes sense. It's good to think of edge cases like this, and we should definitely guard against it. However I'd prefer to fail in this case, since the API shouldn't allow this to happen. Even it happens to be impossible for this case to occur with today's code, future changes may have bugs that create a null or undefined reason that could be silently ignored if we just filter out those reasons. I'd prefer we throw an error if we encounter a null or undefined reason and remove the filterReasons() method entirely, since such a reason existing means that something has gone wrong and the deployment may fail (or worse yet, deploy successfully by chance but with missing dependency logic which may cause it to fail for seemingly no reason in the future).

jusdino · 2022-12-05T21:31:22Z

Thanks for the explanation, this makes sense. It's good to think of edge cases like this, and we should definitely guard against it. However I'd prefer to fail in this case, since the API shouldn't allow this to happen. Even it happens to be impossible for this case to occur with today's code, future changes may have bugs that create a null or undefined reason that could be silently ignored if we just filter out those reasons. I'd prefer we throw an error if we encounter a null or undefined reason and remove the filterReasons() method entirely, since such a reason existing means that something has gone wrong and the deployment may fail (or worse yet, deploy successfully by chance but with missing dependency logic which may cause it to fail for seemingly no reason in the future).

Sounds good. I'll work on that next along with the smaller changes from your last review.

Pull request has been modified.

jusdino · 2022-12-07T16:09:23Z

@comcalvi, @MrArnoldPalmer, I think this covers everything except for what to do about IElement - just need some direction on which way to go with that.

comcalvi · 2022-12-07T22:05:14Z

for IElement, please leave the original union type Element as it was.

comcalvi · 2022-12-10T21:07:23Z

@jusdino Can you please enable contributions by maintainers to your fork? I have a commit I'd like to push to this PR, and then I'll be happy to approve it. Thanks for all your hard work on this PR, this is in a really good spot now!

jusdino · 2022-12-10T23:01:33Z

@jusdino Can you please enable contributions by maintainers to your fork? I have a commit I'd like to push to this PR, and then I'll be happy to approve it. Thanks for all your hard work on this PR, this is in a really good spot now!

Glad to be close to the finish line!

So this PR is from a fork on a GitHub organization and I wasn’t able to find a way to do that with my last PR from there. Last time, I ended up reopening a new PR from a personal fork. I can do that here as well though it will take me a bit - got some personal stuff going on atm.

For reference:
#20419 (comment)

comcalvi · 2022-12-12T19:17:06Z

Thanks! Just link it here and I'll approve once I push a commit to it.

jusdino · 2022-12-17T19:06:41Z

@comcalvi , #23383 is ready 👍

aws-cdk-automation · 2022-12-17T19:26:02Z

AWS CodeBuild CI Report

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

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

comcalvi · 2022-12-19T15:44:07Z

Closing this one in favor of #23383

@comcalvi

Reopening #20419 from a personal fork to allow maintainer edits. @comcalvi 👋 Add some new methods to allow a minimal interface for viewing and editing resource-to-resource dependencies that mirrors the behavior of CfnResource.addDependsOn(). Related to #20418 - details for justification are there ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] 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*

@comcalvi

Reopening aws#20419 from a personal fork to allow maintainer edits. @comcalvi 👋 Add some new methods to allow a minimal interface for viewing and editing resource-to-resource dependencies that mirrors the behavior of CfnResource.addDependsOn(). Related to aws#20418 - details for justification are there ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] 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*

@comcalvi

Reopening aws#20419 from a personal fork to allow maintainer edits. @comcalvi 👋 Add some new methods to allow a minimal interface for viewing and editing resource-to-resource dependencies that mirrors the behavior of CfnResource.addDependsOn(). Related to aws#20418 - details for justification are there ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] 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*

aws-cdk-automation requested a review from a team May 19, 2022 16:43

github-actions bot added the p2 label May 19, 2022

TheRealAmazonKendra changed the base branch from v1-main to main June 2, 2022 09:17

comcalvi previously requested changes Jun 15, 2022

View reviewed changes

jusdino requested a review from comcalvi June 30, 2022 16:03

comcalvi previously requested changes Jul 14, 2022

View reviewed changes

jusdino force-pushed the CfnResource-dependsOn branch from ccfd779 to e066b02 Compare July 18, 2022 21:15

TheRealAmazonKendra previously requested changes Aug 2, 2022

View reviewed changes

jusdino force-pushed the CfnResource-dependsOn branch from c701ed0 to 561f0a6 Compare August 3, 2022 00:22

TheRealAmazonKendra previously requested changes Aug 4, 2022

View reviewed changes

packages/@aws-cdk/core/lib/stack.ts Outdated Show resolved Hide resolved

jusdino force-pushed the CfnResource-dependsOn branch from a24c577 to 65bc9a5 Compare August 8, 2022 23:05

Justin Frahm added 9 commits August 10, 2022 08:17

Add resource/stack dependency methods

11abf8a

Get tests in there and fix the bugs they identify

3c193b7

Address easier review comments

612cdb6

Add CfnResource.replaceDependsOn()

6b0e521

Update, respond to comments

9a16c37

Add README section

6b8811e

Update the _other_ README

d72ec7c

bugfix, integration test

d7b4cff

Add some error/less-common case unit tests

e71c405

jusdino force-pushed the CfnResource-dependsOn branch from 07912f2 to e71c405 Compare August 10, 2022 14:17

TheRealAmazonKendra previously requested changes Aug 12, 2022

View reviewed changes

Naumel requested a review from comcalvi December 1, 2022 08:08

Naumel assigned comcalvi Dec 1, 2022

Justin Frahm added 3 commits December 6, 2022 16:40

Remove filterReasons

3e49f01

Remove StackDependencyReason.description

a6a2153

README cleanup

76f1986

Justin Frahm added 3 commits December 7, 2022 17:21

Remove IElement in favor of Element type

6dbfb68

Merge branch 'main' into CfnResource-dependsOn

18fa180

Merge branch 'main' into CfnResource-dependsOn

508b9b6

jusdino mentioned this pull request Dec 17, 2022

feat(core): CfnResource dependency methods #23383

Merged

4 tasks

Merge branch 'main' into CfnResource-dependsOn

6ed2097

comcalvi closed this Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): CfnResource depends on methods #20419

feat(core): CfnResource depends on methods #20419

jusdino commented May 19, 2022 •

edited

gitpod-io bot commented May 19, 2022

comcalvi left a comment

comcalvi Jun 15, 2022

comcalvi Jul 14, 2022

jusdino Jul 18, 2022

jusdino commented Jul 26, 2022 •

edited

TheRealAmazonKendra left a comment

TheRealAmazonKendra commented Aug 3, 2022

TheRealAmazonKendra left a comment

jusdino commented Aug 8, 2022

jusdino commented Nov 28, 2022

comcalvi commented Nov 29, 2022 •

edited

jusdino commented Nov 29, 2022

comcalvi commented Nov 30, 2022

jusdino commented Dec 5, 2022

comcalvi commented Dec 5, 2022 •

edited

jusdino commented Dec 5, 2022

jusdino commented Dec 7, 2022

comcalvi commented Dec 7, 2022

comcalvi commented Dec 10, 2022

jusdino commented Dec 10, 2022

comcalvi commented Dec 12, 2022

jusdino commented Dec 17, 2022

aws-cdk-automation commented Dec 17, 2022

comcalvi commented Dec 19, 2022

	throw new Error(`Reason for dependency removal is too ambiguous: ${reasonFilter}`);
	throw new Error(`There cannot be more than one reason for dependency removal, found: ${reasonFilter}`);

feat(core): CfnResource depends on methods #20419

feat(core): CfnResource depends on methods #20419

Conversation

jusdino commented May 19, 2022 • edited

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented May 19, 2022

comcalvi left a comment

Choose a reason for hiding this comment

comcalvi Jun 15, 2022

Choose a reason for hiding this comment

comcalvi Jul 14, 2022

Choose a reason for hiding this comment

jusdino Jul 18, 2022

Choose a reason for hiding this comment

jusdino commented Jul 26, 2022 • edited

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

TheRealAmazonKendra commented Aug 3, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

jusdino commented Aug 8, 2022

jusdino commented Nov 28, 2022

comcalvi commented Nov 29, 2022 • edited

jusdino commented Nov 29, 2022

comcalvi commented Nov 30, 2022

jusdino commented Dec 5, 2022

comcalvi commented Dec 5, 2022 • edited

jusdino commented Dec 5, 2022

jusdino commented Dec 7, 2022

comcalvi commented Dec 7, 2022

comcalvi commented Dec 10, 2022

jusdino commented Dec 10, 2022

comcalvi commented Dec 12, 2022

jusdino commented Dec 17, 2022

aws-cdk-automation commented Dec 17, 2022

AWS CodeBuild CI Report

comcalvi commented Dec 19, 2022

jusdino commented May 19, 2022 •

edited

jusdino commented Jul 26, 2022 •

edited

comcalvi commented Nov 29, 2022 •

edited

comcalvi commented Dec 5, 2022 •

edited