fix(CLI): diff --template crashes
#29896
Conversation
github-actions
bot
added
bug
There was a problem hiding this comment.
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.
comcalvi
added
the
pr-linter/exempt-integ-test
| @@ -51,6 +51,10 @@ export function fullDiff( | |||
| isImport?: boolean, | |||
| ): types.TemplateDiff { | |||
|
|
|||
| if (!currentTemplate || !newTemplate) { | |||
| throw new Error('trying to diff a template that is not defined'); | |||
There was a problem hiding this comment.
What does that error even mean? How do I fix this?
| }; | ||
|
|
||
| // THEN | ||
| expect(() => fullDiff(currentTemplate, newTemplate)).toThrow(/trying to diff a template that is not defined/); |
There was a problem hiding this comment.
From this code I can get why there is this check. But how would an actual user end up with this? How can they fix it?
There was a problem hiding this comment.
A user would never end up with this. I added it because diff is design to work on arbitrary json objects, but it achieves this by typing things as any, which allows undefined. undefined breaks it though, so I figured we could be a bit more defensive with that. It's not doing anything useful though, it's just moving the crash earlier, so I can remove this
There was a problem hiding this comment.
The reason this change got through is it looked innocuous and it didn't break any tests, because we had just never tested fixed-template diffs before
|
➡️ PR build request submitted to A maintainer must now check the pipeline and add the |
aws-cdk-automation
added
pr/do-not-merge
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
removed
the
pr-linter/cli-integ-tested
There was a problem hiding this comment.
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
removed
the
pr/do-not-merge
comcalvi
added
the
pr-linter/cli-integ-tested
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
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 CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
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). |
|
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. |
Issue # (if applicable)
Closes #29890.
Reason for this change
cdk diffcrashes with--template.Description of changes
The addition of changeset logic had a leftover refactor that should not have been leftover (trying to pass a template directly instead of a stack artifact). Removes changeset creation code from fixed template mode, which should never create a changeset, and adds a unit test for fixed template diffs so we don't break this in the future.
Description of how you validated changes
unit tests + manual testing. CLI integ tests will be added in a follow-up PR.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license