Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(CLI): change-set diff not required for new stack diffs #29268

Closed
wants to merge 14 commits into from

Conversation

sumupitchayan
Copy link
Contributor

Closes #29265

Adds a check in the fullDiff function to make sure that change-set diff is not run for new stack diffs.


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

Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 26, 2024
@github-actions github-actions bot added the p1 label Feb 26, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team February 26, 2024 20:43
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

Signed-off-by: Sumu <sumughan@amazon.com>
@sumupitchayan sumupitchayan added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Feb 27, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 27, 2024 16:54

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

Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
@sumupitchayan sumupitchayan marked this pull request as ready for review February 27, 2024 18:18
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 27, 2024
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Comments inline. Also did you push this into the testing pipeline yet?

@@ -1231,4 +1231,22 @@ describe('changeset', () => {
expect(differences.resources.differenceCount).toBe(1);
expect(differences.resources.get('BucketResource').changeImpact === ResourceImpact.WILL_IMPORT);
});

test('does not do changeset checks for new stack', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be missing something but I'm not sure how this tests that we do not do a changeset for new stacks. Could you clarify that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was confused on this too - my logic was that calling fullDiff with the current stack being empty (new) should mean that there should be only 1 difference count, and the change impact should be ResourceImpact.WILL_CREATE; I'm not sure how else to test that we don't do changeset for new stacks

packages/aws-cdk/test/diff.test.ts Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Feb 27, 2024
@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.

Signed-off-by: Sumu <sumughan@amazon.com>
Signed-off-by: Sumu <sumughan@amazon.com>
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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 aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Feb 28, 2024
@aws-cdk-automation aws-cdk-automation dismissed their stale review February 28, 2024 18:53

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@scanlonp
Copy link
Contributor

scanlonp commented Mar 8, 2024

Closed by #29394.

@scanlonp scanlonp closed this Mar 8, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
contribution/core This is a PR that came from AWS. p1 pr/needs-cli-test-run This PR needs CLI tests run against it. pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(cli): change-set diff not required for new stack diffs
4 participants