Try to improve code readability of var_kv #581
Conversation
if we skip YAML.unmarshal on the value we may end up with superfluos quotes around the actual value. This is mostly for backwards compability as it may break scripts/manifest that would currently render fine: e.g.: ``` + releases: + - name: bosh + version: "'271.17.0+dev.1638991106'" ``` vs ``` + releases: + - name: bosh + version: 271.17.0+dev.1638885656 ``` Signed-off-by: Brian Cunnie <[email protected]>
|
I added a commit to this PR because we found that the new code also changes rendering behaviour in specific situations |
director/template/var_kv_test.go
Outdated
| Expect(err).ToNot(HaveOccurred()) | ||
| Expect(arg).To(Equal(VarKV{Name: "name", Value: "val"})) | ||
| }) | ||
| It("Trim only removes wrapping quotes", func() { |
There was a problem hiding this comment.
For completeness's sake, can we add in a test or three here that ensures that values with unbalanced quote characters are correctly handled? For example
"'some-data"' or '"some-data'" get transformed into 'some-data" and "some-data', respectively?
I expect this to work today, but it'd be good to ensure that it keeps working in the future.
There was a problem hiding this comment.
There was a syntax issue with the test which I fixed. I wanted to add the suggested cases here but they don't work at the moment. You get errors like:
Unexpected error:
<errors.ComplexError>: {
Err: <*errors.errorString | 0xc00039ac80>{
s: "Deserializing variables 'name='\"some-data'\"'",
},
Cause: <*errors.errorString | 0xc00039ac50>{
s: "yaml: found unexpected end of stream",
},
}
Deserializing variables 'name='"some-data'"': yaml: found unexpected end of stream
occurred
Is this expected to work?
There was a problem hiding this comment.
I think it should since that could be a Passwort Value:
"'someString"' should be untouched and render to "'someString"'
"'someString" should be touched and render to 'someString
I added a "custom" trim instead of using strings.Trim for this.
There was a problem hiding this comment.
ok, now all cases are supported. Ideally, this should be part of a different pr but let's keep it here.
e.g. if a the value is: `"'Password"'` we should return `"'Password"'` thanks @klakin-pivotal for spotting this.
This is a refactoring suggestion for #580. I had hard time to read the code and this is a small refactoring to improve that.