-
Notifications
You must be signed in to change notification settings - Fork 167
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 diff with local yaml templates #541
fix diff with local yaml templates #541
Conversation
Codecov Report
@@ Coverage Diff @@
## master #541 +/- ##
=========================================
+ Coverage 87.47% 87.57% +0.1%
=========================================
Files 95 95
Lines 6114 6123 +9
=========================================
+ Hits 5348 5362 +14
+ Misses 766 761 -5
Continue to review full report at Codecov.
|
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.
Can you add a functional test to tests/suite.bats that reproduces the problem?
stacker/tests/actions/test_diff.py
Outdated
' }\n', | ||
'}\n' | ||
] | ||
self.assertEquals(normalized_template, diff._normalize_json(template)) |
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.
We should just pull _normalize_json
off of the Diff
class and into a function. Easier to test since you don't need to initialize anything.
@ejholmes good thought; helped me catch another error in the display logic. I also moved the normalize_json function out as requested. |
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.
Looks great. Thanks @troyready!
else: | ||
param_diffs = diff_parameters(old_params, new_params) | ||
param_diffs = diff_parameters(old_params, new_params) | ||
if param_diffs: |
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.
Ah, I noticed this bug recently as well. Thanks for fixing!
…emplate fix diff with local yaml templates
#540 fixed the generally broken diffs in #530 , but introduced a problem with yaml templates.