Conversation
In place of 'coreos/yaml' Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
krnowak
left a comment
There was a problem hiding this comment.
The normalization is the icky part. I'm not sure if this won't cause some nasty bugs.
config/config.go
Outdated
| // Normalize transform the "-" in "_" for the keys | ||
| // in the YAML configuration. | ||
| // e.g reboot-strategy -> reboot_strategy |
There was a problem hiding this comment.
| // Normalize transform the "-" in "_" for the keys | |
| // in the YAML configuration. | |
| // e.g reboot-strategy -> reboot_strategy | |
| // Normalize transforms dashes into underlines for the keys | |
| // in the YAML configuration. | |
| // e.g reboot-strategy -> reboot_strategy |
config/config.go
Outdated
| // Normalize transform the "-" in "_" for the keys | ||
| // in the YAML configuration. | ||
| // e.g reboot-strategy -> reboot_strategy | ||
| func Normalize(r io.Reader) string { |
There was a problem hiding this comment.
Honestly, this is a rather risky function. Will it mess some inline bash script like:
- some_inline_stuff: |
echo 'foo-bar: stuff'https://play.golang.com/p/u6ogWtIHI9Q
This kind of transform is probably done safe only by actually parsing YAML.
There was a problem hiding this comment.
I tend to agree here. If we mutate the source yaml, we're asking for trouble. It would be nice if the yaml package supported multiple tags for the same field, but sadly it does not.
There was a problem hiding this comment.
I agree with all of you. I tried to provide a "best effort" approach to continue maintaining the current behavior but honestly I'd be in favor of dropping this "compatibility" (e.g reboot-strategy == reboot_strategy). I understand it was quite convenient back in the days, but today it's a nonsense to me: with a clear spec, such a behavior would not be needed. I never seen Kubernetes YAML manifest or other well known YAML configuration doing such a thing.
This best effort is just to provide backward compatibility with folks still using coreos-cloudinit with "bad" keys.
There was a problem hiding this comment.
Personally, I'd drop this backward compatibility then, because continuing to support invalid keys adds too much to maintenance overhead. Otherwise we risk mangling unrelated stuff in config file. If you decide to drop it then we also will need to announce it. I think a changelog entry when bumping the commit ID in scripts repo would be sufficient.
There was a problem hiding this comment.
Thanks. Then I dropped the compatibility.
config/validate/node.go
Outdated
| v := v.(map[interface{}]interface{}) | ||
| for k, cv := range v { | ||
| cn := node{name: fmt.Sprintf("%s", k)} | ||
| c, ok := findKey(cn.name, c) | ||
| if ok { | ||
| cn.line = c.lineNumber | ||
| cpv := v | ||
| v, ok := cpv.(map[interface{}]interface{}) | ||
| if ok { | ||
| for k, cv := range v { | ||
| cn := node{name: fmt.Sprintf("%s", k)} | ||
| c, ok := findKey(cn.name, c) | ||
| if ok { | ||
| cn.line = c.lineNumber | ||
| } | ||
| toNode(cv, c, &cn) | ||
| n.children = append(n.children, cn) | ||
| } | ||
| toNode(cv, c, &cn) | ||
| n.children = append(n.children, cn) | ||
| } else { | ||
| sv := cpv.(map[string]interface{}) | ||
| for k, cv := range sv { | ||
| cn := node{name: fmt.Sprintf("%s", k)} | ||
| c, ok := findKey(cn.name, c) | ||
| if ok { | ||
| cn.line = c.lineNumber | ||
| } | ||
| toNode(cv, c, &cn) | ||
| n.children = append(n.children, cn) | ||
| } | ||
|
|
There was a problem hiding this comment.
You probably could avoid this duplication by putting the common code into an inline function (excuse my rusty go, haven't typed that in a long time):
handleKV := func(k,v interface{}) {
cn := node{name: fmt.Sprintf("%s", k)}
c, ok := findKey(cn.name, c)
if ok {
cn.line = c.lineNumber
}
toNode(cv, c, &cn)
n.children = append(n.children, cn)
}
cpv := v
v, ok := cpv.(map[interface{}]interface{})
if ok {
for k, cv := range v {
handleKV(k, cv)
}
} else {
sv := cpv.(map[string]interface{})
for k, cv := range sv {
handleKV(k, cv)
}
}Probably could be made even shorter by using Go's generics, but I have no experience with those.
config/validate/validate.go
Outdated
| yaml.UnmarshalMappingKeyTransform = func(nameIn string) (nameOut string) { | ||
| return nameIn | ||
| } | ||
| cfg = []byte(config.Normalize(bytes.NewReader(cfg))) |
There was a problem hiding this comment.
Hm, this normalizes the yaml config now, whereas the old code had a no-op transform at this point. Not sure if it matters, though.
Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
dbc3713 to
88f93b2
Compare
some of these tests were relying on the fact that the unmarshal of the configuration does not panic but it's against yaml lib definition: if the lib can not unmarshal, it will just panic. we also drop tests regarding hyphen -> underscore conversion: this support is removed. Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
Signed-off-by: Mathieu Tortuyaux <mtortuyaux@microsoft.com>
88f93b2 to
a5dc429
Compare
github.com/coreos/yamlis not even maintained, let's switch to the upstream.This drops:
write-files->write_files)reboot-strategy: [1, 2, 3])Related to: flatcar/Flatcar#1271