fix(cli): warns about missing `--no-rollback` flag that is present #32309

rix0rrr · 2024-11-27T14:02:38Z

The logic on rollback and !rollback was 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

The logic was inverted :(

aws-cdk-automation

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.

codecov · 2024-11-27T14:14:43Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.36%. Comparing base (512cf95) to head (01dfa6b).
Report is 1 commits behind head on main.

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

Flag	Coverage Δ
suite.unit	`78.36% <100.00%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`78.36% <100.00%> (+0.02%)`	⬆️

dsturnbull · 2024-11-27T22:36:05Z

packages/aws-cdk/test/api/deploy-stack.test.ts

+  [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'],


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.

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)?"

aws-cdk-automation · 2024-11-28T14:37:45Z

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

mrgrain · 2024-11-28T14:42:33Z

packages/aws-cdk/lib/api/aws-auth/sdk.ts

@@ -951,7 +951,6 @@ export class SDK {
        });
        const command = new GetCallerIdentityCommand({});
        const result = await client.send(command);
-        debug(result.Account!, result.Arn, result.UserId);


Why remove?

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.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2024-11-28T16:57:06Z

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 · 2024-11-28T17:27:27Z

AWS CodeBuild CI Report

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

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

mergify · 2024-11-28T17:27:43Z

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

github-actions · 2024-11-28T17:27:57Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

fix: CLI warns about missing --no-rollback flag that is present

023a4fa

The logic was inverted :(

rix0rrr requested a review from a team as a code owner November 27, 2024 14:02

github-actions bot added bug This issue is a bug. p2 labels Nov 27, 2024

aws-cdk-automation requested a review from a team November 27, 2024 14:02

rix0rrr requested a deployment to test-pipeline November 27, 2024 14:03 — with GitHub Actions Waiting

mergify bot added the contribution/core This is a PR that came from AWS. label Nov 27, 2024

aws-cdk-automation previously requested changes Nov 27, 2024

View reviewed changes

aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Nov 27, 2024

fix tests

b9e2027

rix0rrr requested a deployment to test-pipeline November 27, 2024 15:16 — with GitHub Actions Waiting

Typo

36b4d62

rix0rrr requested a deployment to test-pipeline November 27, 2024 15:18 — with GitHub Actions Waiting

ashishdhingra mentioned this pull request Nov 27, 2024

AwsCdkCli: cdk deploy tries automatic rollback even when --no-rollback specified #32295

Closed

1 task

dsturnbull reviewed Nov 27, 2024

View reviewed changes

rix0rrr added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Nov 28, 2024

rix0rrr changed the title ~~fix: CLI warns about missing --no-rollback flag that is present~~ fix(cli): warns about missing --no-rollback flag that is present Nov 28, 2024

github-actions bot added the effort/small Small work item – less than a day of effort label Nov 28, 2024

Add another test case for the coverage

bf0bb5e

rix0rrr deployed to test-pipeline November 28, 2024 14:36 — with GitHub Actions Active

mrgrain reviewed Nov 28, 2024

View reviewed changes

mrgrain approved these changes Nov 28, 2024

View reviewed changes

rix0rrr added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Nov 28, 2024

aws-cdk-automation removed the pr/needs-cli-test-run This PR needs CLI tests run against it. label Nov 28, 2024

Merge branch 'main' into huijbers/inverted-logic

01dfa6b

mergify bot requested a deployment to test-pipeline November 28, 2024 16:58 Waiting

mergify bot merged commit 559d676 into main Nov 28, 2024
16 of 17 checks passed

mergify bot deleted the huijbers/inverted-logic branch November 28, 2024 17:27

github-actions bot locked as resolved and limited conversation to collaborators Nov 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): warns about missing `--no-rollback` flag that is present #32309

fix(cli): warns about missing `--no-rollback` flag that is present #32309

rix0rrr commented Nov 27, 2024 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Nov 27, 2024 •

edited

Loading

dsturnbull Nov 27, 2024

rix0rrr Nov 28, 2024

aws-cdk-automation commented Nov 28, 2024

mrgrain Nov 28, 2024

rix0rrr Nov 28, 2024

mergify bot commented Nov 28, 2024

aws-cdk-automation commented Nov 28, 2024

mergify bot commented Nov 28, 2024

github-actions bot commented Nov 28, 2024

fix(cli): warns about missing --no-rollback flag that is present #32309

fix(cli): warns about missing --no-rollback flag that is present #32309

Conversation

rix0rrr commented Nov 27, 2024 • edited Loading

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 27, 2024 • edited Loading

Codecov Report

dsturnbull Nov 27, 2024

Choose a reason for hiding this comment

rix0rrr Nov 28, 2024

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 28, 2024

mrgrain Nov 28, 2024

Choose a reason for hiding this comment

rix0rrr Nov 28, 2024

Choose a reason for hiding this comment

mergify bot commented Nov 28, 2024

aws-cdk-automation commented Nov 28, 2024

AWS CodeBuild CI Report

mergify bot commented Nov 28, 2024

github-actions bot commented Nov 28, 2024

fix(cli): warns about missing `--no-rollback` flag that is present #32309

fix(cli): warns about missing `--no-rollback` flag that is present #32309

rix0rrr commented Nov 27, 2024 •

edited

Loading

codecov bot commented Nov 27, 2024 •

edited

Loading