Skip to content

Fix parameter handling for diffs#540

Merged
ejholmes merged 2 commits into
cloudtools:masterfrom
acmcelwee:amc-fix-diff
Feb 22, 2018
Merged

Fix parameter handling for diffs#540
ejholmes merged 2 commits into
cloudtools:masterfrom
acmcelwee:amc-fix-diff

Conversation

@acmcelwee

@acmcelwee acmcelwee commented Feb 21, 2018

Copy link
Copy Markdown
Member

After using the DAG RC for the past week or so, I realized diffs were broken today. All of the param portions of the diffs came out as something like this:

--- Old Parameters
+++ New Parameters
******************
-Owner = foo
+Owner = unused_value
-Environment = d
+Environment = unused_value

This also led to a lot of unresolved variable errors like:

[2018-02-20T22:34:42] Variable "ASGMapping" in blueprint "ECSHosts" is missing
Traceback (most recent call last):
  File "/Users/adam/code/stacker/stacker/plan.py", line 86, in _run_once
    status = self.fn(self.stack, status=self.status)
  File "/Users/adam/code/stacker/stacker/actions/diff.py", line 228, in _diff_stack
    new_template = stack.blueprint.to_json()
  File "/Users/adam/code/stacker/stacker/blueprints/base.py", line 495, in to_json
    self.resolve_variables(variables_to_resolve)
  File "/Users/adam/code/stacker/stacker/blueprints/base.py", line 445, in resolve_variables
    self.name
  File "/Users/adam/code/stacker/stacker/blueprints/base.py", line 209, in resolve_variable
    raise MissingVariable(blueprint_name, var_name)
MissingVariable: Variable "ASGMapping" in blueprint "ECSHosts" is missing

@codecov-io

codecov-io commented Feb 21, 2018 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #540 into master will decrease coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #540      +/-   ##
==========================================
- Coverage   87.53%   87.51%   -0.03%     
==========================================
  Files          94       94              
  Lines        6072     6077       +5     
==========================================
+ Hits         5315     5318       +3     
- Misses        757      759       +2
Impacted Files Coverage Δ
stacker/actions/diff.py 57.03% <0%> (ø) ⬆️
stacker/tests/fixtures/mock_blueprints.py 62.06% <60%> (-0.2%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2eafbaa...2cc8fcc. Read the comment docs.

ejholmes
ejholmes requested changes Feb 21, 2018
View reviewed changes

@ejholmes ejholmes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you rebase this in top of b795e47? That commit adds a failing test for the diff command.

Comment thread stacker/actions/diff.py Outdated
new_params = dict()
for p in parameters:
new_params[p['ParameterKey']] = p['ParameterValue']
new_template = stack.blueprint.to_json(new_params)

@ejholmes ejholmes Feb 21, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is the correct behavior. to_json takes a dict of variables, not necessarily parameters. This change would make the behavior in _diff_stack significantly different from _launch_stack. I think it's important that these use similar code paths or we're gonna run into these types of problems.

I think the root of the problem is the change from blueprint.rendered to blueprint.to_json in 4d0fad8#diff-1a5240c955da6d0e576a399c23c44953L215. to_json apparently has too many side effects to really rely on it here.

I think this should probably just be switched back to using rendered, which is what we use in _launch_stack. It would mean diffing a JSON template against a YAML template will be garbage, but I think that's fine. Thoughts @troyready?

@acmcelwee acmcelwee Feb 21, 2018

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think calling .rendered sounds like right thing to do. (bfe0343)

RE: diffing YAML against JSON. On a related note, I was just talking to a coworker yesterday about adding support to stacker to use YAML as the final representation instead of JSON. IMO, diffs of yaml are probably a bit cleaner, anyway. I'd be happy to work on adding that as a config option, if others thought it would be useful.

@acmcelwee acmcelwee changed the title Provide new params to json formatter for diffs Fix parameter handling for diffs Feb 21, 2018
@acmcelwee

acmcelwee commented Feb 21, 2018

Copy link
Copy Markdown
Member Author

@ejholmes rebased and commit history cleaned up

@troyready

troyready commented Feb 21, 2018

Copy link
Copy Markdown
Contributor

Good catch; sorry for my part in starting this pain :)

Agreed on dropping to_json usage in diff. A case could be made to keep it if we were going to do it on both the old & new template, but I don't think anyone is going to lose sleep over a massive diff if they change a template from json -> yaml.

ejholmes
ejholmes approved these changes Feb 22, 2018
View reviewed changes

@ejholmes ejholmes left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! Thanks @acmcelwee and @troyready!

@ejholmes ejholmes merged commit 3a3326f into cloudtools:master Feb 22, 2018
@acmcelwee acmcelwee deleted the amc-fix-diff branch February 22, 2018 00:10
@troyready troyready mentioned this pull request Feb 23, 2018
Merged
phrohdoh pushed a commit to phrohdoh/stacker that referenced this pull request Dec 18, 2018
@ejholmes
Merge pull request cloudtools#540 from acmcelwee/amc-fix-diff
0be3f15 
Fix parameter handling for diffs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ejholmes ejholmes ejholmes approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@acmcelwee @codecov-io @troyready @ejholmes