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

feat(CLI): improved nested stack diff #29172

Merged
merged 54 commits into from
Mar 13, 2024
Merged

Conversation

comcalvi
Copy link
Contributor

@comcalvi comcalvi commented Feb 19, 2024

Issue # 27238

Reason for this change

The existing nested stack diff places a fake property, NestedTemplate, in the templates to be diffed. This prevents displaying resource replacement information in the diff, like we do for top level stacks. This PR does not add changeset replacement information from changesets, but it does add replacement information from the spec.

Description of changes

Reworked nested stack diff to treat nested stacks as top level stacks. This improves the visual UX and sets us up for using changesets with nested stacks.

Before

Screenshot 2024-02-19 at 1 47 59 PM

After

Screenshot 2024-02-19 at 1 48 48 PM

Description of how you validated changes

Unit tests + manual tests.

Checklist


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

… just in case this other idea doesn't work"

This reverts commit 10c86cc.
… save it just in case this other idea doesn't work""

This reverts commit e1e4d34.
…far, but save it just in case this other idea doesn't work"""

This reverts commit f793503.
…oach so far, but save it just in case this other idea doesn't work""""

This reverts commit 26eeb68.
…ong approach so far, but save it just in case this other idea doesn't work"""""

This reverts commit 7725f60.
…y the wrong approach so far, but save it just in case this other idea doesn't work""""""

This reverts commit aa3e4c7.
… probably the wrong approach so far, but save it just in case this other idea doesn't work"""""""

This reverts commit 61ff29f.
…o delete the NestedTemplate thing and store the nested templates some other way
This reverts commit 61ff29f.
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 20, 2024 16:26

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

@aws-cdk-automation aws-cdk-automation added pr/needs-maintainer-review This PR needs a review from a Core Team Member and removed pr/needs-cli-test-run This PR needs CLI tests run against it. labels Feb 20, 2024
Copy link
Contributor

@colifran colifran left a comment

Choose a reason for hiding this comment

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

LGTM -- nice job!

Copy link
Contributor

mergify bot commented Feb 21, 2024

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 aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 21, 2024
export interface TemplateWithNestedStackCount {
readonly deployedTemplate: Template;
readonly nestedStackCount: number;
export interface RootTemplateWithNestedStacks {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can RootTemplateWithNestedStacks and NestedStackTemplates not be the same structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided not to because the functions that use this interface as a return type only get the generated templates of nested stacks, not the top level root stack. The functions that take these objects as input have a different way of getting the top-level generated template (namely, they take a StackArtifact that contains it). They need something special that the StackArtifact provides, so I decided to keep the generated template out of this data structure, but only for the top-level artifacts.

We could mark generatedTemplate as optional, but that's less clear than using the separate type.

@comcalvi comcalvi force-pushed the comcalvi/improved-nested-stack-diff branch from 5e00582 to 2c9dd9e Compare March 13, 2024 19:01
@aws-cdk-automation
Copy link
Collaborator

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

Copy link
Contributor

mergify bot commented Mar 13, 2024

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 135b520 into main Mar 13, 2024
11 of 12 checks passed
@mergify mergify bot deleted the comcalvi/improved-nested-stack-diff branch March 13, 2024 20:20
Copy link
Contributor

mergify bot commented Mar 13, 2024

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

vinayak-kukreja pushed a commit that referenced this pull request Mar 14, 2024
reverts #29394, which prevented changeset creation during `cdk diff` if
a stack did not exist. The lookup of the stack to check its existence is
failing for customers that have CI/CD that won't assume the deploy role
when running CDK diff.

Long-term fix: delete the stack if it didn't exist before we created the
changeset, but wait for its state to reach `DELETE_COMPLETE` to avoid
problems with subsequent commands.

Preserves changes from #29172

----

*By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache-2.0 license*
obraafo pushed a commit to obraafo/aws-cdk that referenced this pull request Apr 1, 2024
### Issue # (if applicable)


### Reason for this change

The existing nested stack diff places a fake property, `NestedTemplate`, in the templates to be diffed. This prevents displaying resource replacement information in the diff, like we do for top level stacks. This PR does *not* add changeset replacement information from changesets, but it does add replacement information from the spec. 

### Description of changes

Reworked nested stack diff to treat nested stacks as top level stacks. This improves the visual UX and sets us up for using changesets with nested stacks. 

#### Before

<img width="957" alt="Screenshot 2024-02-19 at 1 47 59 PM" src="https://github.com/aws/aws-cdk/assets/66279577/a94275c4-e7c3-4d2c-a924-ee61c36bea4d">


#### After
<img width="957" alt="Screenshot 2024-02-19 at 1 48 48 PM" src="https://github.com/aws/aws-cdk/assets/66279577/5263aaf9-ef2f-4228-b413-81e780c4b8f8">



### Description of how you validated changes

Unit tests + manual tests.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested pr-linter/exempt-integ-test The PR linter will not require integ test changes pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants