merge.go: drop Compression field if nil in child #1329
Conversation
If a child's Compression field is nil, since it may have uncompressed Source that gets copied to result, Compression should not be set in result even if it's set in parent Fixes https://github.com/coreos/ignition/issues/1327
| @@ -240,6 +240,10 @@ func mergeStruct(parent reflect.Value, parentPath path.ContextPath, child reflec | |||
| resultField.Set(mergeStruct(parentField.Elem(), parentFieldPath, childField.Elem(), childFieldPath, resultFieldPath, transcript).Addr()) | |||
| transcribeOne(parentFieldPath, resultFieldPath, transcript) | |||
| transcribeOne(childFieldPath, resultFieldPath, transcript) | |||
| case kind == reflect.Ptr && childField.IsNil() && fieldMeta.Name == "Compression": | |||
There was a problem hiding this comment.
Is there a better place to put this? Doesn't seem like it should be a case in mergeStruct() since it's such a general function but I couldn't find where else to put it
There was a problem hiding this comment.
I'm confused how this works...wouldn't we fall through to the case below without this?
There was a problem hiding this comment.
Yeah it would fall through to the next case which calls resultField.Set(parentField). Afaict that's what we want to avoid doing, and instead we want to do nothing (ie set Compression to nil)
There was a problem hiding this comment.
The bug we're hitting is when parentField has Compression = gzip but childField.IsNil()
There was a problem hiding this comment.
Right sorry, I get confused sometimes how in Go fallthrough requires an explicit fallthrough.
There was a problem hiding this comment.
Haha when I read your comment I had a definite moment of crisis over that
| {path.New(TAG_CHILD, "storage", "files", 0, "path"), path.New(TAG_RESULT, "storage", "files", 0, "path")}, | ||
| {path.New(TAG_CHILD, "storage", "files", 0, "contents", "source"), path.New(TAG_RESULT, "storage", "files", 0, "contents", "source")}, | ||
| {path.New(TAG_PARENT, "storage", "files", 0, "contents"), path.New(TAG_RESULT, "storage", "files", 0, "contents")}, | ||
| {path.New(TAG_CHILD, "storage", "files", 0, "contents"), path.New(TAG_RESULT, "storage", "files", 0, "contents")}, | ||
| {path.New(TAG_PARENT, "storage", "files", 0), path.New(TAG_RESULT, "storage", "files", 0)}, | ||
| {path.New(TAG_CHILD, "storage", "files", 0), path.New(TAG_RESULT, "storage", "files", 0)}, | ||
| {path.New(TAG_PARENT, "storage", "files"), path.New(TAG_RESULT, "storage", "files")}, | ||
| {path.New(TAG_CHILD, "storage", "files"), path.New(TAG_RESULT, "storage", "files")}, | ||
| {path.New(TAG_PARENT, "storage"), path.New(TAG_RESULT, "storage")}, | ||
| {path.New(TAG_CHILD, "storage"), path.New(TAG_RESULT, "storage")}, |
There was a problem hiding this comment.
I reverse engineered this so have no idea if it's correct
|
Thanks for the PR. Config merging is defined to be field-by-field, so the current behavior is the correct one. This PR would break existing configs where the child config intends to inherit the parent's Butane's automatic compression semantics were implemented without a full recognition of the implications for config merging. The fix should be in Butane; see discussion in coreos/butane#332 (comment). |
See coreos/butane#341 This obviates coreos/ignition#1329 which we are carrying currently.
If a child's Compression field is nil, since it may have uncompressed
Source that gets copied to result, Compression should not be set in
result even if it's set in parent
Fixes coreos/butane#332