Permit --var values to contain newlines. #580
Conversation
- yaml.Unmarshall mangles newlines in non-yaml partials. This commit checks the return value type and, if string (what appears to be returned by yaml.Unmarshal for invalid YAML input), simply passes through the original string value. - Although this is a behavior change, it is not expected that any user would be depending on this behavior. [#180138088] Signed-off-by: Rajan Agaskar <ragaskar@vmware.com>
| @@ -33,7 +33,11 @@ func (a *VarKV) UnmarshalFlag(data string) error { | |||
| return bosherr.WrapErrorf(err, "Deserializing variables '%s'", data) | |||
| } | |||
|
|
|||
| *a = VarKV{Name: pieces[0], Value: vars} | |||
| if _, ok := vars.(string); ok { | |||
There was a problem hiding this comment.
I'm pretty bad at Go, but this is weird enough that it should have a comment explaining what it's doing and why it's doing it. I can guess, but I definitely wouldn't be confident in my guess without a lot of research.
There was a problem hiding this comment.
I think the technical term for this is a type assertion
There was a problem hiding this comment.
sure, the commit message serves as an explanation, but a comment can be added as a convenience.
There was a problem hiding this comment.
This must be a lie, right?
> If i does not hold a T, the statement will trigger a panic.
Nevermind, I'm an idiot that hasn't had coffee yet... that's when you use the variant that doesn't return an error status.
director/template/var_kv_test.go
Outdated
| @@ -56,5 +56,13 @@ var _ = Describe("VarKV", func() { | |||
| Expect(val3).To(Equal(map[interface{}]interface{}{"key31": "str"})) | |||
| }) | |||
| }) | |||
|
|
|||
| Context("When key/value has newlines", func() { | |||
There was a problem hiding this comment.
Might as well specify in the context that the string is not a yaml
- Eliminates the need for future developers to do git archaeology. Signed-off-by: Rajan Agaskar <ragaskar@vmware.com>
rkoster
requested review from
a team,
jpalermo and
beyhan
and removed request for
a team
|
Enough people have looked at this, gonna merge, Thanks @ragaskar |
|
I somehow missed that I was assigned as reviewer to this pr. I have created a small refactoring suggestion in #581. @selzoc and @klakin-pivotal as you have been involved in the discussion here could you provide your feedback for #581. |
checks the return value type and, if string (what appears to be
returned by yaml.Unmarshal for invalid YAML input), simply passes
through the original string value.
user would be depending on this behavior.
[#180138088]
Signed-off-by: Rajan Agaskar ragaskar@vmware.com