gruntwork-io / boilerplate

A tool for generating files and folders ("boilerplate") from a set of templates
https://www.gruntwork.io
Mozilla Public License 2.0
158 stars 12 forks source link

Fix issue with unmarshaling yaml maps #67

Closed bwhaley closed 3 years ago

bwhaley commented 3 years ago

This works around an issue in the yaml package that causes YAML maps to be unable to be marshalled to JSON.

What was happening was that unmarshaling a map variable like this:

SomeMap:
  SomeKey: SomeVal

would result in a variable of type map[interface{}]interface{} rather than map[string]interface{}. The former cannot be marshaled to JSON correctly since the JSON package (and the JSON spec) require that keys are strings. Hence you could not marshal the map to JSON in a boilerplate template like this:

{{ .SomeMap | toPrettyJson }}

where toPrettyJson is a function in sprig. It would instead just render an empty string.

bwhaley commented 3 years ago

We could alternatively maintain a fork of go-yaml with a single line change, similar to what this person has done.

bwhaley commented 3 years ago

Thanks for the review. A nasty shave indeed, but it seemed necessary for the problem I'm working on.

Unfortunately, the last version I pushed when I opened the PR didn't quite work. The behavior was not the same as the native Unmarshal for unexpected conditions like an empty string or invalid YAML. I've worked around this in the most recent commit and tests seem to pass now. Mind taking another look?

bwhaley commented 3 years ago

Actually, hold off on the review, it's still not right. 👎

bwhaley commented 3 years ago

Okay, now it should be ready for another round of review!

bwhaley commented 3 years ago

Nope, still finding other errors with the variants of unmarshaling. I'll need to write a more abstract unmarshaler that handles different types. This only works for map[string]interface{}.

bwhaley commented 3 years ago

Finally got to the bottom of it with the latest update. Convert() still needs unit tests.

bwhaley commented 3 years ago

Added unit test for Convert() func.

bwhaley commented 3 years ago

All good suggestions. Updated and incorporated in 6e977ec.

bwhaley commented 3 years ago

Thanks for the helpful review!