Use goccy/go-yaml for v1 / Prep bringing back go-yaml v2 for v0.x#604
Use goccy/go-yaml for v1 / Prep bringing back go-yaml v2 for v0.x#604
Conversation
mumoshu
force-pushed
the
goccy-go-yaml
branch
from
99421a8 to
97e81fc
Compare
Ref #435 Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu
force-pushed
the
goccy-go-yaml
branch
from
97e81fc to
2cc8fce
Compare
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
…2 for v0.x Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
| releases: | ||
| - name: myrelease1 | ||
| chart: mychart1 | ||
| installed: no |
There was a problem hiding this comment.
It turned out on/off and yes/no are deprecated in YAML 1.2.
goccy/go-yaml#214
To make it backward-compatible for Helmfile v.0x, I'd like to revert Helmfile v0.x to use gopkg.in/yaml.v2 by default.
Let's feature the deprecation in the helmfile v1 proposal and enable goccy/go-yaml only for Helmfile v1, so that we won't break anything for Helmfile v0.x while we can still migrate to a maintained yaml library.
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
|
@mumoshu all is ok. I fixed the test cases. please review again |
|
where is then |
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: yxxhero <aiopsclub@163.com>
| return yaml.Marshal(hsa) | ||
| } | ||
|
|
||
| func (hs *HelmState) UnmarshalYAML(value *yaml.Node) error { |
There was a problem hiding this comment.
@yxxhero I believe you can revert 589576e and make it back to use UnmarshalYaml(func(interface{}) error) error, as that might the equivalent for this function in gopkg.in/yaml.v2 and goccy/go-yaml https://github.com/goccy/go-yaml/blob/6cdefc42e112ac71cbe316e1eed264ea62f58e25/yaml.go#L49-L52
There was a problem hiding this comment.
HelmState's UnmarshalYAML was created for v1, and I feel like it can be adapted to the latest goccy/go-yaml interface, which I don't need to introduce for v0. @mumoshu
There was a problem hiding this comment.
I've updated my comment before you replied! Sorry for the confusion. Regardless of when it was introduced first, I think the problem of using the new interface seems to have been caught by the test failure- it now does not indent nested YAML dicts correctly.
There was a problem hiding this comment.
yes. should we post an issue to goccy/go-yaml?
There was a problem hiding this comment.
Why? We don't need to stick with MarshalYAML() ([]byte, error) { nor UnmarshalYAML(b []byte) error, right?
Or do you know anything that isn't covered by our current test suite?
There was a problem hiding this comment.
Apparently goccy's respect strict parsing setting even you used InterfaceUnmarshaller to unmarshal nested structs. See https://github.com/goccy/go-yaml/blob/6cdefc42e112ac71cbe316e1eed264ea62f58e25/decode.go#L626 and https://github.com/goccy/go-yaml/blob/6cdefc42e112ac71cbe316e1eed264ea62f58e25/decode.go#L1045-L1050.
It would be great if we could add a new test case to test the desired behavior though... Any idea about that?
There was a problem hiding this comment.
yeah. I think so. when use UnmarshalYAML(b []byte) error for HelmState. we need a strict mode to keep the same behavior with the yaml.UnMarshal.
| // helmStateAlias is helm state alias | ||
| type helmStateAlias HelmState | ||
|
|
||
| func (hs HelmState) MarshalYAML() (interface{}, error) { |
There was a problem hiding this comment.
@yxxhero Same for MarshalYAML. goccy/go-yaml does support InterfaceMarshaller and expect the MarshalYAML function to return a Go object that is then serialized into YAML rather than a raw byte array that is embedded into the resulting YAML document as-is.
https://github.com/goccy/go-yaml/blob/6cdefc42e112ac71cbe316e1eed264ea62f58e25/yaml.go#L28-L31
There was a problem hiding this comment.
HelmState's MarshalYAML was created for v1, and I feel like it can be adapted to the latest goccy/go-yaml interface, which I don't need to introduce for v0.
There was a problem hiding this comment.
Thanks for your reply! See #604 (comment)
There was a problem hiding this comment.
@yxxhero Have you seen some tests failing due to diffs like the below?
Diff:
--- Expected
+++ Actual
@@ -17,6 +17,6 @@
metadata:
- name: foo-2
- namespace: default
+name: foo-2
+namespace: default
data:
- bar: BAR
+bar: BAR
---
@@ -26,6 +26,6 @@
metadata:
- name: foo-1
- namespace: default
+name: foo-1
+namespace: default
data:
- foo: FOO
+foo: FOO
I think this indicates that MarshalYAML changes from 589576e is unnecessary.
There was a problem hiding this comment.
yeah. I saw it. and I tried to solve this issue. but it's because goccy/yaml logic issue. I already post an issue for this.
Signed-off-by: yxxhero <aiopsclub@163.com>
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
mumoshu
force-pushed
the
goccy-go-yaml
branch
from
d323cc1 to
f34117b
Compare
Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
|
@yxxhero Thank you so much for your throughout review and confirmation ❤️ |
2 participants
This is a successor to #596. We need a smooth migration path from
gopkg.in/yaml.v2, and this pull request moves it forward withgoccy/go-yamlinstead ofgopkg.in/yaml.v3. Merging this unblocks users stuck in Helmfile v0.146.x or earlier due to #435, so that they can upgrade to 0.147.x or greater without updating their helmfile configs.We previously tried to upgrade to
yaml.v3(#394) in Helmfile v0.x, presuming it won't break anything. Apparently, it broke use-cases where you want to layer release'svaluesfield over three or more release templates and releases (#435).We then tried to bring back
yaml.v2for Helmfile v0.x and keepyaml.v3for the upcoming Helmfile v1. However, it failed due to incompatibility in the Unmarshaller interface betweenyaml.v2andyaml.v3(#596).goccy/go-yamlis, from my observation, a well-maintained alternative toyaml.v2. One of its premises is that it enables us to swap the implementation fromgopkg.in/yaml.v2togoccy/go-yamljust by replacing the import directive. It seems to use the sameUnmarshallerinterface as yaml.v2 too.Once this PR gets merged, I'd like to follow-up with adding a new build-time variable and an envvar to set the proper default for the yaml parser Helmfile uses and the ability to switch the parser at runtime. All in all, the next Helmfile release, v0.150.0 will get reverted to use
gopkg.in/yaml.v2by default which resolves #435.New users who started using Helmfile since any of v0.148.0, v0.148.1, and v0.149.0 might be already relying on the new behavior, They might need to specify a new envvar to enable
goccy/go-yaml.Ref #435
Signed-off-by: Yusuke Kuoka ykuoka@gmail.com