-
Notifications
You must be signed in to change notification settings - Fork 4k
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(cli): report errors from resource failures in nested stacks #27318
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.
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.
one minor comment, but overall this looks ready to merge. Nice work!
|
||
if (event.ResourceType === 'AWS::CloudFormation::Stack' && !CFN_SUCCESS_STATUS.includes(event.ResourceStatus ?? '')) { | ||
// If the event is not for `this` stack, recursively call for events in the nested stack | ||
if (event.PhysicalResourceId !== stackToPollForEvents && event.LogicalResourceId !== stackToPollForEvents) { |
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.
Why are we checking for both here? PhysicalResourceId
and LogicalResourceId
should never equal each other, and stackToPollForEvents
can only ever contain one. It looks we're assigning the value of PhysicalResourceId
, so why do we compare against LogicalResourceId
?
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.
yeah I had just PhysicalResourceId
earlier and then got confused with this. Reverted it.
aws-cdk/packages/aws-cdk/lib/api/util/cloudformation/stack-activity-monitor.ts
Lines 545 to 547 in 4e8c9c4
if (failure.event.StackName === failure.event.LogicalResourceId) { | |
continue; | |
} |
93eef99
to
61245d5
Compare
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
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). |
fix(cli): report errors from resource failures in nested stacks.
Description
Currently
StackActivityMonitor
usesreadNewEvents()
method to constantly poll CFN to get the latest deployment updates. However it only does it for the root stack and the resources in the root stack. If one of the resource in the root stack is another nested stack and one of the resource in that nested stack fails, CFN does not propagate or copy the error message in the nested stack failure rather it's a genericEmbedded stack <stackArn> was not successfully updated
This PR updates the
readNewEvents()
to recursively poll for events from the nested stack deployments as well. If errors are detected in the nested stack events, they are added to bothStackActivityMonitor:errors
as well as added in thePrinter
.Following is a before/after this change. We are deploying RootStack -> Nested Stack -> AppSync Resolver and the AppSync resolver fails with the error
Only one resolver is allowed per field
in CFN.Before this change
After this change
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license