feat(CLI): synth displays "AssertDescription: CDK bootstrap stack version 6 required" by comcalvi · Pull Request #31092 · aws/aws-cdk

comcalvi · 2024-08-12T22:48:14Z

Issue # (if applicable)

Reason for this change

The CDK CLI shows the stack template, which includes the CFN Rule CheckBootstrapVersion. This rule will fail a deployment if the bootstrap is not right. Customers think this rule is an error message.

Description of changes

Obscure this CheckBootstrapVersion Rule from the template when we print it, if it exists. If it is the only Rule, remove the Rules section entirely.

Description of how you validated changes

Manual testing, unit tests, and CLI integration tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

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.

mrgrain

This seems like not a great idea to me to obscure things from the template the we later deploy regardless. What other options where considered as a solution?

Feel free to dismiss my review if the has been discussed sufficiently in your team.

comcalvi · 2024-08-12T23:29:15Z

@mrgrain the team has not discussed this yet. I think this is the best path forward, and it's a precedent we've set by obscuring AWS::CDK::Metadata from the diff.

I see the following options:

Update our docs to tell users it's expected when you run synth or diff.
Update the error message, telling people they can ignore it if they see it in CLI output.
Obscure it from the CLI output, a precedent we set with AWS::CDK::Metadata.

I don't think the first two address the issue.

For 1: People will likely run synth once before they read whatever doc tells them about it, and be confused.
For 2: The error message will become more confusing if they get it during deployment and it says "you can maybe ignore this."

3 addresses the issue by ensuring people only see the error message where they need to see it; if a deployment is failing because of it. If people want to see the full, un-altered diff, they can pass --strict, which will keep it there.

mrgrain · 2024-08-12T23:36:52Z

Hmm I've forget about metadata, my bad. That's a great argument for it. --strict is also nice, although it's not a great name for what it apparently does. Carry on. 😁

There were a few other ideas in the issue:

slightly adjust the message
move it to the top of the template
print "success" to stderr

I also wonder if this check needs to be in the template or if it could be done by the cli.

comcalvi · 2024-08-14T20:30:37Z

@mrgrain that's a good point. The reason this is a Rule in the template is that deployments are sometimes initiated from pipelines, without the CDK CLI; this is documented here: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/core/lib/stack-synthesizers/stack-synthesizer.ts#L309

mrgrain · 2024-08-15T17:27:51Z

@mrgrain that's a good point. The reason this is a Rule in the template is that deployments are sometimes initiated from pipelines, without the CDK CLI; this is documented here: https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/core/lib/stack-synthesizers/stack-synthesizer.ts#L309

Cool! This is probably true, but if you are curious let's dig deeper. How are pipelines deploying stacks? What do they need the Bootstrap stack for?

My guess here is that it's the cdk-asset command that really cares about this. But I'm not sure.

Either way, this is not a blocker.

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

scanlonp

Approving with 1 nit.

We should update the cli option description here:

aws-cdk/packages/aws-cdk/lib/cli.ts

Line 263 in 9acd528

    
           .option('strict', { type: 'boolean', desc: 'Do not filter out AWS::CDK::Metadata resources or mangled non-ASCII characters', default: false })

.

scanlonp · 2024-08-16T00:00:35Z

packages/aws-cdk/lib/cdk-toolkit.ts

+    // see https://github.com/aws/aws-cdk/issues/17942
+    if (template.Rules.CheckBootstrapVersion) {
+      if (Object.keys(template.Rules).length > 1) {
+        delete template.Rules.CheckBootstrapVersion;


I thought you had mentioned that the rules would only be deleted if the check bootstrap version was the only rules. Did I misinterpret that, or did we switch to always removing the bootstrap version check?

The intention is to always remove the Rule, but to remove the entire Rules section if that is the only rule.

aws-cdk-automation · 2024-08-16T16:31:49Z

➡️ 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.

non-blocking

comcalvi · 2024-08-16T16:33:09Z

@mrgrain I am curious but I'm not able to dig deeper into this right now.

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.

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

mergify · 2024-08-16T23:43:22Z

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-08-17T00:13:15Z

AWS CodeBuild CI Report

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

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

mergify · 2024-08-17T00:13:35Z

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-08-17T00:13:52Z

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.

template obscurement

9cc1a01

aws-cdk-automation requested a review from a team August 12, 2024 22:48

github-actions bot added bug This issue is a bug. p1 labels Aug 12, 2024

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

comcalvi added the pr/do-not-merge This PR should not be merged at this time. label Aug 12, 2024

comcalvi temporarily deployed to test-pipeline August 12, 2024 22:49 — with GitHub Actions Inactive

obscure

ac059d6

aws-cdk-automation previously requested changes Aug 12, 2024

View reviewed changes

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

comcalvi had a problem deploying to test-pipeline August 12, 2024 22:50 — with GitHub Actions Failure

mrgrain previously requested changes Aug 12, 2024

View reviewed changes

obscure diff

a5cc246

comcalvi had a problem deploying to test-pipeline August 12, 2024 23:16 — with GitHub Actions Failure

cli-integ

a68d7cb

comcalvi temporarily deployed to test-pipeline August 13, 2024 21:18 — with GitHub Actions Inactive

readme

e66e5c4

comcalvi added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Aug 14, 2024

comcalvi temporarily deployed to test-pipeline August 14, 2024 21:26 — with GitHub Actions Inactive

github-actions bot added the effort/medium Medium work item – several days of effort label Aug 14, 2024

integ test

ab81896

comcalvi temporarily deployed to test-pipeline August 15, 2024 19:27 — with GitHub Actions Inactive

comcalvi changed the title ~~feat(CLI): CLI displays "AssertDescription: CDK bootstrap stack version 6 required"~~ feat(CLI): synth displays "AssertDescription: CDK bootstrap stack version 6 required" Aug 15, 2024

comcalvi added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Aug 15, 2024

scanlonp approved these changes Aug 16, 2024

View reviewed changes

CLI option text

e959a42

comcalvi temporarily deployed to test-pipeline August 16, 2024 16:31 — with GitHub Actions Inactive

comcalvi added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested labels Aug 16, 2024

aws-cdk-automation previously requested changes Aug 16, 2024

View reviewed changes

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

comcalvi added pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested and removed pr/do-not-merge This PR should not be merged at this time. pr/needs-cli-test-run This PR needs CLI tests run against it. labels Aug 16, 2024

Merge branch 'main' into comcalvi/bootstarp

d9f84e5

mergify bot had a problem deploying to test-pipeline August 16, 2024 23:44 Failure

mergify bot merged commit 751a922 into main Aug 17, 2024

mergify bot deleted the comcalvi/bootstarp branch August 17, 2024 00:13

github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2024

Conversation

comcalvi commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

mrgrain left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comcalvi commented Aug 12, 2024

Uh oh!

mrgrain commented Aug 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comcalvi commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrgrain commented Aug 15, 2024

Uh oh!

scanlonp left a comment

Choose a reason for hiding this comment

Uh oh!

scanlonp Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

comcalvi Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Aug 16, 2024

Uh oh!

comcalvi commented Aug 16, 2024

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 16, 2024

Uh oh!

aws-cdk-automation commented Aug 17, 2024

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 17, 2024

Uh oh!

github-actions bot commented Aug 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

comcalvi commented Aug 12, 2024 •

edited

Loading

mrgrain left a comment •

edited

Loading

mrgrain commented Aug 12, 2024 •

edited

Loading

comcalvi commented Aug 14, 2024 •

edited

Loading