Skip to content
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 issue with unmarshaling yaml maps #67

Merged
merged 6 commits into from
Oct 30, 2020
Merged

Conversation

bwhaley
Copy link
Contributor

@bwhaley bwhaley commented Oct 29, 2020

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.

  • Needs a unit test

@bwhaley
Copy link
Contributor Author

bwhaley commented Oct 29, 2020

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

yorinasub17
yorinasub17 previously approved these changes Oct 29, 2020
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM! What a nasty yak shave!

variables/yaml_mapstr.go Outdated Show resolved Hide resolved
variables/yaml_mapstr.go Outdated Show resolved Hide resolved
@bwhaley
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

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
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

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

@bwhaley
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

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

@bwhaley
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

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
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

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

@bwhaley
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

Added unit test for Convert() func.

yorinasub17
yorinasub17 previously approved these changes Oct 30, 2020
Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM! I had a few nits about code style and defensive coding, but feel free to ignore/punt as these are minor issues IMO.

variables/yaml_helpers.go Outdated Show resolved Hide resolved
variables/yaml_helpers.go Outdated Show resolved Hide resolved
variables/yaml_helpers_test.go Show resolved Hide resolved
variables/yaml_helpers.go Outdated Show resolved Hide resolved
@bwhaley
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

All good suggestions. Updated and incorporated in 6e977ec.

Copy link
Contributor

@yorinasub17 yorinasub17 left a comment

Choose a reason for hiding this comment

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

LGTM!

@bwhaley
Copy link
Contributor Author

bwhaley commented Oct 30, 2020

Thanks for the helpful review!

@bwhaley bwhaley merged commit c274f0f into master Oct 30, 2020
@bwhaley bwhaley deleted the custom-yaml-unmarshal branch October 30, 2020 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants