Fix parameter handling for diffs

[acmcelwee]

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 Report

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

@@            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]

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

[ejholmes]

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]

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]

@ejholmes rebased and commit history cleaned up

[troyready]

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]

Looks good! Thanks @acmcelwee and @troyready!