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

chore(aws-cdk): Revert "fix(aws-cdk): include nested stacks when building changesets … #19618

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

madeline-k
Copy link
Contributor

…(#19494)"

This reverts commit 97cc8e2.

Reverting this commit since it introduced a regression. Or, it is not a regression but the regression runs of the CLI integ tests think it is. The same test fails in the run-against-latest-release and the run-against-latest-code regression test suites: https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/test/integ/cli/cli.integtest.ts#L629.

Test fails with:

    expect(received).not.toEqual(expected) // deep equality

    Expected: not "arn:aws:cloudformation:ap-southeast-2:416588550161:changeSet/cdk-deploy-change-set/c8c68622-fc38-4199-81b2-b74206380152"
      at cli.integtest.ts:644:38
          at runMicrotasks (<anonymous>)
      at ../helpers/cdk.ts:130:7
      at ResourcePool.using (../helpers/resource-pool.ts:44:14)
      at ../helpers/test-helpers.ts:38:14

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use cdk-integ to deploy the infrastructure and generate the snapshot (i.e. cdk-integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Mar 29, 2022

@github-actions github-actions bot added the p2 label Mar 29, 2022
@madeline-k madeline-k changed the title Revert "fix(aws-cdk): include nested stacks when building changesets … chore(aws-cdk): Revert "fix(aws-cdk): include nested stacks when building changesets … Mar 29, 2022
@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2022

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 29, 2022
@madeline-k madeline-k requested a review from a team March 29, 2022 21:25
@mergify
Copy link
Contributor

mergify bot commented Mar 29, 2022

Thank you for contributing! Your pull request will be updated from master 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: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 090b158
  • 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 bbc2628 into aws:master Mar 29, 2022
@flyingImer
Copy link
Contributor

@madeline-k curious - what's the next step for this to roll out again? do you have a issue link to track?

@madeline-k
Copy link
Contributor Author

Hey @flyingImer, I just re-opened the original issue that this PR closed #19224. We will track it there.

StevePotter pushed a commit to StevePotter/aws-cdk that referenced this pull request Apr 27, 2022
…ding changesets … (aws#19618)

…(aws#19494)"

This reverts commit 97cc8e2.

Reverting this commit since it introduced a regression. Or, it is not a regression but the regression runs of the CLI integ tests think it is. The same test fails in the run-against-latest-release and the run-against-latest-code regression test suites: https://github.com/aws/aws-cdk/blob/master/packages/aws-cdk/test/integ/cli/cli.integtest.ts#L629.

Test fails with:
```
    expect(received).not.toEqual(expected) // deep equality

    Expected: not "arn:aws:cloudformation:ap-southeast-2:416588550161:changeSet/cdk-deploy-change-set/c8c68622-fc38-4199-81b2-b74206380152"
      at cli.integtest.ts:644:38
          at runMicrotasks (<anonymous>)
      at ../helpers/cdk.ts:130:7
      at ResourcePool.using (../helpers/resource-pool.ts:44:14)
      at ../helpers/test-helpers.ts:38:14
```
----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)?
	* [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)?

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

gagipro commented Apr 30, 2023

any news on that ?

@skinny85
Copy link
Contributor

I thought #18207 fixed this, so that cdk diff now works for nested stacks?

@gagipro
Copy link

gagipro commented May 1, 2023

I thought #18207 fixed this, so that cdk diff now works for nested stacks?

I don't think so. You cannot have nested stack details in your changeset with --no-execute.

#19224 (comment)

I managed to make it work with the help of this link, but should be included since long time !!!

@skinny85
Copy link
Contributor

skinny85 commented May 1, 2023

But why do you need to review changes in the changeset, if you have a working cdk diff command?

@gagipro
Copy link

gagipro commented May 1, 2023

But why do you need to review changes in the changeset, if you have a working cdk diff command?

because everything is in an automated pipeline. A request for a change is made and someone as to review later and validate or reject the change.

and that's the way CloudFormation do work.

anyway : by patching the code : it do work as intended, see url I posted.

This needs to be adressed to behave the same as CloudFormation

@skinny85
Copy link
Contributor

skinny85 commented May 1, 2023

Can't they review the output of cdk diff? Isn't that much nicer than looking at ChangeSet diffs, which contain a lot of noise from what I remember?

@gagipro
Copy link

gagipro commented May 1, 2023

Can't they review the output of cdk diff? Isn't that much nicer than looking at ChangeSet diffs, which contain a lot of noise from what I remember?

Try it.

Cdk diff : you need cli and direct Access.
Validate a changeset is in the console, red or green graphical interface that permits to inspect dozens of sub nested stacks.

I will now open a ticket through enterprise support to see if they Can prioritize this issue.

I tried a patched deploy-stack.js aws-cdk lib and it seems to be ok.

So I'm wondering why they don't implement.

Probable you know if it breaks something else if you are aws cdk dev.

@konstantinj
Copy link

@gagipro I've tried through enterprise already but please try as well.
@skinny85 IMO cdk diff is different since it will not fail fast. Example: For some change a new resource needs to be created but there is already a resource with that name. Creating a changeset with CFN will fail then before any change to the stack as a whole will hpapen. With cdk diff I won't see that and my stack update will fail during the actual update and needs to roll back. In production. That's not ideal.

@gagipro
Copy link

gagipro commented May 2, 2023

@gagipro I've tried through enterprise already but please try as well. @skinny85 IMO cdk diff is different since it will not fail fast. Example: For some change a new resource needs to be created but there is already a resource with that name. Creating a changeset with CFN will fail then before any change to the stack as a whole will hpapen. With cdk diff I won't see that and my stack update will fail during the actual update and needs to roll back. In production. That's not ideal.

Hi @konstantinj

Enterprise support already replied to check with devs on github.

    "sed -i 's/            ResourcesToImport: this.options.resourcesToImport,/            IncludeNestedStacks: true,\\n            ResourcesToImport: this.options.resourcesToImport,/g' /usr/local/lib/node_modules/aws-cdk/lib/api/deploy-stack.js",
    "sed -i 's/ResourcesToImport:this.options.resourcesToImport,/IncludeNestedStacks:true,ResourcesToImport:this.options.resourcesToImport,/g' /usr/local/lib/node_modules/aws-cdk/lib/index.js"

@skinny85
Copy link
Contributor

skinny85 commented May 4, 2023

@skinny85 IMO cdk diff is different since it will not fail fast. Example: For some change a new resource needs to be created but there is already a resource with that name. Creating a changeset with CFN will fail then before any change to the stack as a whole will hpapen. With cdk diff I won't see that and my stack update will fail during the actual update and needs to roll back. In production. That's not ideal.

Thanks for the explanation @konstantinj - I actually did not know ChangeSets worked this way.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants