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

fix: add a dependsOn to custom resource for auth trigger #11504

Merged

Conversation

danielleadams
Copy link
Contributor

@danielleadams danielleadams commented Dec 1, 2022

Description of changes

This adds a dependsOn for an auth category resource: Custom Resource has a dependency on Lambda trigger and its IAM resources, so they should be deployed synchronously.

Issue #, if available

The issue was identified by @awsluja, who has been investigating broken/flaky tests.

Description of how you validated changes

Snapshot test unit for this behavior.

Checklist

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

@danielleadams danielleadams requested a review from a team as a code owner December 1, 2022 00:13
@danielleadams danielleadams marked this pull request as draft December 1, 2022 00:24
serviceToken: authTriggerFn.functionArn,
properties: { userpoolId: userpoolId.valueAsString, lambdaConfig: authTriggerConnections, nonce: uuid() },
resourceType: 'Custom::CustomAuthTriggerResourceOutputs',
});

customResource.node.addDependency(authTriggerFn);
Copy link
Contributor

Choose a reason for hiding this comment

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

should we also add the policy from line 114 as a dependency, just in case?
It looks like the custom resource might already be waiting on the authTriggerFn, but not the policy
image

Copy link
Contributor Author

@danielleadams danielleadams Dec 1, 2022

Choose a reason for hiding this comment

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

That dependsOn will add all the dependencies of the dependency added... I'll add a test to make it clearer.

@danielleadams
Copy link
Contributor Author

Failing test is unrelated

@danielleadams danielleadams marked this pull request as ready for review December 1, 2022 22:24
@danielleadams
Copy link
Contributor Author

sobolk
sobolk previously approved these changes Dec 1, 2022
awsluja
awsluja previously approved these changes Dec 1, 2022
Copy link
Contributor

@awsluja awsluja left a comment

Choose a reason for hiding this comment

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

awesome!

@danielleadams danielleadams dismissed stale reviews from awsluja and sobolk via f1575fc December 2, 2022 14:40
@danielleadams
Copy link
Contributor Author

@sobolk @awsluja I had to fix the unit test - do you mind another review?

@codecov-commenter
Copy link

Codecov Report

Merging #11504 (f1575fc) into dev (284c264) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##              dev   #11504   +/-   ##
=======================================
  Coverage   49.87%   49.87%           
=======================================
  Files         698      698           
  Lines       33791    33792    +1     
  Branches     6929     6929           
=======================================
+ Hits        16852    16854    +2     
+ Misses      15405    15404    -1     
  Partials     1534     1534           
Impacted Files Coverage Δ
...dformation/utils/generate-auth-trigger-template.ts 100.00% <100.00%> (ø)
...li/src/domain/amplify-usageData/getUsageDataUrl.ts 100.00% <0.00%> (+12.50%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@danielleadams danielleadams merged commit 1ee69a4 into aws-amplify:dev Dec 2, 2022
@danielleadams danielleadams deleted the add-lambda-depends-on-custom-auth branch December 2, 2022 21:00
sobolk pushed a commit to sobolk/amplify-cli that referenced this pull request Dec 12, 2022
…#11504)

* fix: add a dependsOn to custom resource for auth trigger

* test: add snapshot tests to show cfn changes for dependencies

* fix: mock uuid lib in test
sobolk added a commit that referenced this pull request Dec 12, 2022
* fix: add a dependsOn to custom resource for auth trigger (#11504)

* fix: add a dependsOn to custom resource for auth trigger

* test: add snapshot tests to show cfn changes for dependencies

* fix: mock uuid lib in test

* fix: auth trigger template rendering

Co-authored-by: Danielle Adams <6271256+danielleadams@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants