-
Notifications
You must be signed in to change notification settings - Fork 4k
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
fix(cli): diff against multiple stacks do not always fail if any have a diff #7690
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for putting this fix together! I think we should also add an integ test here. We recently converted the integ tests to be in jest, so it should be a bit easier to add one now.
Currently, the diff command only remembers if there are any diffs with the last stack that was diffed. So, if all of your stacks have a diff except for the last one, the diff command would exit successfully even if told to fail on diff. This fix corrects the behavior so that if any of the stacks have a diff, then the exit code would be non-zero if configured to do so. fixes aws#7492
Co-authored-by: Shiv Lakshminarayan <[email protected]>
Rebased the PR |
That might be a little tricky for me. I don't feel comfortable running integration tests against my work AWS account. Feel like I got lucky getting unit tests working and figured out hah @shivlaks are you interesting in writing the integration test? |
@polothy sure thing, I'll see if I can pick that up later this week and bring this one home |
Thanks for the assist and for the review! |
@shivlaks any chance we can get this going again? 😄 |
@polothy thanks for the gentle prod - writing up that integ test this week |
@shivlaks wondering if we can just merge as is? I'm not really seeing the advantage of integration test, because it isn't a problem with the diffing mechanism, just the flag for keeping track of changes over X stacks. |
@polothy i'll try to get it in ahead of the next release. i'm less worried about the integ test for this specific change, but would like it to be there to prevent a regression in the future. |
❤️
Ah, gotchya. Thanks again for commenting back and helping! |
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 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Yah! Thanks so much! |
… a diff (aws#7690) ### Commit Message Currently, the diff command only remembers if there are any diffs with the last stack that was diffed. So, if all of your stacks have a diff except for the last one, the diff command would exit successfully even if told to fail on diff. This fix corrects the behavior so that if any of the stacks have a diff, then the exit code would be non-zero if configured to do so. fixes aws#7492 ### End Commit Message ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Commit Message
Currently, the diff command only remembers if there are any diffs
with the last stack that was diffed. So, if all of your stacks have
a diff except for the last one, the diff command would exit
successfully even if told to fail on diff.
This fix corrects the behavior so that if any of the stacks have
a diff, then the exit code would be non-zero if configured to do so.
fixes #7492
End Commit Message
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license