fix(cli): warns about missing --no-rollback flag that is present
#32309
Conversation
The logic was inverted :(
rix0rrr
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
There was a problem hiding this comment.
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.
aws-cdk-automation
added
the
pr/needs-cli-test-run
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32309 +/- ##
==========================================
+ Coverage 78.34% 78.36% +0.02%
==========================================
Files 105 105
Lines 7170 7170
Branches 1315 1315
==========================================
+ Hits 5617 5619 +2
+ Misses 1365 1364 -1
+ Partials 188 187 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
rix0rrr
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
rix0rrr
requested a deployment
to
test-pipeline
GitHub Actions
Waiting
| [StackStatus.UPDATE_FAILED, 'no-rollback', 'no-replacement', 'did-deploy-stack'], | ||
| [StackStatus.UPDATE_FAILED, 'no-rollback', 'replacement', 'failpaused-need-rollback-first'], | ||
| [StackStatus.UPDATE_FAILED, 'rollback', 'replacement', 'failpaused-need-rollback-first'], | ||
| [StackStatus.UPDATE_COMPLETE, 'no-rollback', 'replacement', 'replacement-requires-rollback'], |
There was a problem hiding this comment.
I haven't traced through the code fully, but this seems to read that replacement-requires-rollback, will successfully complete when a replacement is needed but we passed in no-rollback.
There was a problem hiding this comment.
When the stack is in a stable state (UPDATE_COMPLETE), the user passes --no-rollback, and there is a replacement to do, the deployStack function will say: replacement-requires-rollback.
That is to say, the combination of replacement and no-rollback is not possible. This is a CloudFormation limitation.
What will happen in practice is that our CLI will catch this return code and ask "Do you want to perform a regular (--rollback) deployment (y/n)?"
rix0rrr
added
the
pr-linter/exempt-integ-test
rix0rrr
changed the title
--no-rollback flag that is present--no-rollback flag that is present
github-actions
bot
added
the
effort/small
rix0rrr
temporarily deployed
to
test-pipeline
GitHub Actions
Inactive
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
| @@ -951,7 +951,6 @@ export class SDK { | |||
| }); | |||
| const command = new GetCallerIdentityCommand({}); | |||
| const result = await client.send(command); | |||
| debug(result.Account!, result.Arn, result.UserId); | |||
There was a problem hiding this comment.
Pretty sure this is a debug line left by Otavio :).
In any case, I don't want just the account number, ARN and userId printed to the log without text.
rix0rrr
added
the
pr-linter/cli-integ-tested
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
removed
the
pr/needs-cli-test-run
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
The logic on
rollbackand!rollbackwas inverted in a couple of places, causing the check to be the wrong way around and making reasoning about these options harder.Revert the logic and do a more comprehensive test suite around these options.
Also remove a stray
debug()statement that was left in from a previous PR, and show the stack status in the error message.Closes #32295.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license