Use gopkg.in/yaml.v2 for Helmfile v0.x by mumoshu · Pull Request #609 · helmfile/helmfile

mumoshu · 2022-12-28T01:20:22Z

This should fix #435 for Helmfile v0.x releases since the next v0.150.0.
We introduce a new envvar to opt-in to the new YAML library, so that you can give it a shot before upgrading your Helmfile to v1. The same envvar can be used to opt-out of the new YAML library after you upgrade to Helmfile v1, giving you a more flexible migration story.

Signed-off-by: Yusuke Kuoka [email protected]

Ref #435 Signed-off-by: Yusuke Kuoka <[email protected]>

Signed-off-by: Yusuke Kuoka <[email protected]>

…v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

…lmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

… for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

…yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

…pkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

… Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu · 2022-12-29T08:30:53Z

@yxxhero FYI, it seems like goccy/go-yaml is more strict when parsing {{ ... }} at the dict value position:

TEST: happypath - simple rollout of httpbin chart
INFO: Diffing test/integration/test-cases/happypath/input/happypath.yaml
in test/integration/test-cases/happypath/input/happypath.yaml: failed to read happypath.yaml: reading document at index 1: [28:20] unexpected mapping key
      25 |     chart: incubator/raw
      26 |     version: 0.2.3
      27 |     values:
    > 28 |       - mysecret: {{ .Environment.Values.mysecret }}
                              ^
      29 |       - values.yaml
      30 | 
      31 |   - name: helmx
      32 |     
FAIL: unexpected exit code returned by helmfile diff

I'm going to update - mysecret: {{ .Environment.Values.mysecret }} to - mysecret: "{{ .Environment.Values.mysecret }}" so that it's considered a valid YAML both by yaml.v2 and goccy/go-yaml.
Probably we might better document about the change in the v1 proposal too?

mumoshu · 2022-12-29T08:34:19Z

@yxxhero NVM. Apparently, it is due to that v1 mode requires .gotmpl ext for helmfile.yaml to enable the templating feature. We might need to rename every integration test input.yaml to input.yaml.gotmpl or come up with another way to make the integration test suite runnable for both helmfile v0 and v1 builds...

yxxhero · 2022-12-29T08:41:46Z

@mumoshu I agreed with you so much.

yxxhero · 2022-12-29T08:43:38Z

@yxxhero FYI, it seems like goccy/go-yaml is more strict when parsing {{ ... }} at the dict value position:
TEST: happypath - simple rollout of httpbin chart
INFO: Diffing test/integration/test-cases/happypath/input/happypath.yaml
in test/integration/test-cases/happypath/input/happypath.yaml: failed to read happypath.yaml: reading document at index 1: [28:20] unexpected mapping key
      25 |     chart: incubator/raw
      26 |     version: 0.2.3
      27 |     values:
    > 28 |       - mysecret: {{ .Environment.Values.mysecret }}
                              ^
      29 |       - values.yaml
      30 | 
      31 |   - name: helmx
      32 |     
FAIL: unexpected exit code returned by helmfile diff
I'm going to update - mysecret: {{ .Environment.Values.mysecret }} to - mysecret: "{{ .Environment.Values.mysecret }}" so that it's considered a valid YAML both by yaml.v2 and goccy/go-yaml. Probably we might better document about the change in the v1 proposal too?

it's more conform to a standard of yaml spec. right? @mumoshu

… fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu · 2022-12-29T08:45:37Z

@yxxhero Right. And it seems like every yaml implementation (yaml.v2, yaml.v3, and goccy/go-yaml) seems to fail if we tried to parse that. The failure was due to that I missed .gotmpl ext for helmfile v1 integration test.

yxxhero · 2022-12-29T08:49:09Z

@mumoshu yeah. you are right. we should update the ext of integration test files.

… fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

… fixup! fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu · 2022-12-29T12:22:12Z

+         # Helmfile v1
+         - helm-version: v3.10.3
+           kustomize-version: v4.5.7
+           plugin-secrets-version: 4.0.0
+           plugin-diff-version: 3.6.0
+           extra-helmfile-flags:
+           v1mode: "true"


@yxxhero We now run the integration test suite for the v1 mode once per build, with the representative versions of helmfile dependencies, so that we won't break any v1 functionality until we finally stop maintaining v0.

mumoshu · 2022-12-29T12:22:42Z

        HELMFILE_HELM3: 1
        TERM: xterm
        EXTRA_HELMFILE_FLAGS: ${{ matrix.extra-helmfile-flags }}
+        HELMFILE_V1MODE: ${{ matrix.v1mode }}


FYI, HELMFILE_V1MODE is defined in pkg/runtime/runtime.go

mumoshu · 2022-12-29T12:24:01Z

+		v := runtime.GoccyGoYaml
+		runtime.GoccyGoYaml = tc.goccyGoYaml
+		t.Cleanup(func() {
+			runtime.GoccyGoYaml = v
+		})


This is an idiom I used widely across many test sources to ensure we modify the GoccyGoYaml runtime variable only while running the (sub-)test.

mumoshu · 2022-12-29T12:25:24Z

 	}

-	t.Run("fail due to known field", func(t *testing.T) {
+	t.Run("fail due to unknown field with goccy/go-yaml", func(t *testing.T) {


You might also see this idiom, the same test is run with goccy/goyaml and with yaml.v2, so that we are sure that we don't break anything while migrating to goccy/goyaml or helmfile v1.

mumoshu · 2022-12-29T12:27:41Z

 	UpgradeNoticeDisabled         = "HELMFILE_UPGRADE_NOTICE_DISABLED"
 	V1Mode                        = "HELMFILE_V1MODE"
+	GoccyGoYaml                   = "HELMFILE_GOCCY_GOYAML"
+	CacheHome                     = "HELMFILE_CACHE_HOME"


This isn't strictly necessary to change our yaml library. However I added this because this helped tidy up our e2e/snapshot test to be runnable with/without v1 mode.
The envvar is used in pkg/remote/remote.go and set in snapshot_test.go

mumoshu · 2022-12-29T12:28:24Z

 	}
+
+	// You can switch the YAML library at runtime via an envvar:
+	switch os.Getenv(envvar.GoccyGoYaml) {


Although goccy/go-yaml is the default for v1, you can opt in/out in both v0 and v1, by setting this envvar.

mumoshu · 2022-12-29T12:30:26Z

 				}

-				var deserialized map[interface{}]interface{}
+				var deserialized map[string]interface{}


yaml.v2 deserializes nested yaml dict to map[interface{}]interface{} while yaml.v3 and goccy/go-yaml serializes it to map[string]interface{}.
My original design was to ensure any nested dict is deserialized to the latter, by calling maputil.CastKeysToStrings

mumoshu · 2022-12-29T12:30:46Z

 				return nil, fmt.Errorf("failed executing template expressions in release \"%s\".values[%d] = \"%s\": %v", r.Name, i, ts, err)
 			}
 			result.Values[i] = s.String()
+		case map[interface{}]interface{}:


https://github.com/helmfile/helmfile/pull/609/files#r1058925709

mumoshu · 2022-12-29T12:30:56Z

 				Name:      "app",
 				Namespace: "dev",
-				Values:    []interface{}{map[interface{}]interface{}{"key": "app-val0"}},
+				Values:    []interface{}{map[string]interface{}{"key": "app-val0"}},


https://github.com/helmfile/helmfile/pull/609/files#r1058925709

mumoshu · 2022-12-29T12:33:14Z

 		return nil, fmt.Errorf("%s, offending yaml: %s", err, str)
 	}
+
+	m, err := maputil.CastKeysToStrings(m)


https://github.com/helmfile/helmfile/pull/609/files#r1058925709

This is especially important here because this function is supposed to be used like fromYaml | toJson within helmfile templates. Go's json pkg which powers toJson is unable to serialize map[interface{}]interface{} deserialized by yaml.v2. This CastKeysToStrings call normalizes it so that both yaml.v2 and goccy/go-yaml implementation of FromYaml is compatible with ToJson.

mumoshu · 2022-12-29T12:34:55Z

 }

 func TestHelmfileTemplateWithBuildCommand(t *testing.T) {
+	t.Run("with goccy/go-yaml", func(t *testing.T) {


We now run all the snapshot tests for both goccy/go-yaml and yaml.v2

mumoshu · 2022-12-29T12:36:15Z


+			tmpDir := t.TempDir()
+			// HELM_CACHE_HOME contains downloaded chart archives
+			helmCacheHome := filepath.Join(tmpDir, "helm_cache")


I had to give clean HELM_CACHE_HOME and HELM_CONFIG_HOME to every snapshot test case to make it not affected by the previous tests.

mumoshu · 2022-12-29T12:38:18Z

-skipping missing secrets file matching "git::https://github.com/helmfile/helmfile.git@test/e2e/template/helmfile/testdata/snapshot/pr_560/secrets.yaml?ref=main"
 Templating release=foo, chart=../../charts/raw-0.1.0
-exec: helm template foo ../../charts/raw-0.1.0 --values /tmp/helmfile/foo-values-d459bc67c --debug
-helm> install.go:192: [debug] Original chart version: ""


I had to disable debug logging for this test case because running it with yaml.v2 resulted in adding an extra helm> line here due to an unknown reason.
It looked harmless but hard to solve so I just disabled the debug log to make it stable.

mumoshu · 2022-12-29T12:40:54Z

    values:
    - template1: template1
    valuesTemplate:
-    - template1Label: "{{` '{{ .Release.Labels.template1 }}' `}}"


I'm not completely sure, but goccy/go-yaml seemed to translate single quotes enclosed in double quotes to triple single quotes, which resulted in this test case failing.
You don't need the single quotes anyway to write a valid helmfile yaml for this configuration so I just omitted it.

mumoshu · 2022-12-29T12:43:03Z

  inheritedBaseLabel: base
  template2: template2
  template2Label: template2
-  templates:


yaml.v2 encoded this in the flow style but we are not interested in a specific yaml library to encode the same go map to a yaml dict in flow style or non-flow style. I just removed this key out of values to make the output look the same between yaml.v2 and goccy/go-yaml. See input.yaml for the change.

mumoshu · 2022-12-29T12:44:09Z

+  config_file="helmfile.yaml"
+  if [[ ${HELMFILE_V1MODE} = true ]]; then
+    pushd "${chart_need_case_input_dir}"
+    mv "${config_file}" "${config_file}.gotmpl"
+    config_file="${config_file}.gotmpl"
+    popd
+  fi


This is a new idiom I used in all the integration test runner script to rename helmfile.yaml to helmfile.yaml.gotmpl only when it's run with v1 mode.

mumoshu · 2022-12-29T12:46:18Z

@yxxhero DCO is failing due to perhaps a few commits missed signoffs but I'm too lazy to fix it. It would be great if we could handle it by using the squash-and-merge option and including the signoff line in the squashed commit.

yxxhero · 2023-01-01T01:51:46Z

@mumoshu why test/integration/test-cases/yaml-overwrite/input/issue.657.yaml.gotmpl be renamed?

mumoshu · 2023-01-02T20:37:32Z

@yxxhero That's because it's supposed work only with goccy/go-yamlm and therefore helmfile v1, which requires .gotmpl. Remember that we now run the integration test suite against both helmfile v0 and v1, and that we run the yaml-overwrite test only for v1!

yxxhero · 2023-01-04T00:21:08Z

@yxxhero That's because it's supposed work only with goccy/go-yamlm and therefore helmfile v1, which requires .gotmpl. Remember that we now run the integration test suite against both helmfile v0 and v1, and that we run the yaml-overwrite test only for v1!

got.

mumoshu · 2023-01-04T09:16:45Z

@yxxhero Thanks for your review! Let's see how this works in the wild.

mumoshu requested review from itscaro, xiaomudk and yxxhero as code owners December 28, 2022 01:20

mumoshu force-pushed the go-back-to-gopkgin-yaml-v2-for-helmfile-v0.x branch 5 times, most recently from af1880c to ca9da1d Compare December 28, 2022 03:46

yxxhero reviewed Dec 28, 2022

View reviewed changes

Comment thread pkg/yaml/yaml.go

Use gopkg.in/yaml.v2 for Helmfile v0.x

cf0d5b9

Ref #435 Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu force-pushed the go-back-to-gopkgin-yaml-v2-for-helmfile-v0.x branch from ca9da1d to cf0d5b9 Compare December 28, 2022 08:43

mumoshu added 3 commits December 28, 2022 09:13

fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x

299d504

fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x

a3d5f27

Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x

d12c768

Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu force-pushed the go-back-to-gopkgin-yaml-v2-for-helmfile-v0.x branch from 1a19e6d to d12c768 Compare December 29, 2022 00:07

mumoshu added 7 commits December 29, 2022 00:47

fixup! fixup! fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x

fb8f40e

Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile …

397b5e7

…v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! Use gopkg.in/yaml.v2 for He…

98b1550

…lmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! fixup! Use gopkg.in/yaml.v2…

e95c08b

… for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! Use gopkg.in/…

619c204

…yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! Use go…

756e830

…pkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…

bea859c

… Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…

cc24502

… fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…

74e5358

… fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup! fixup!…

f4af106

… fixup! fixup! fixup! Use gopkg.in/yaml.v2 for Helmfile v0.x Signed-off-by: Yusuke Kuoka <[email protected]>

mumoshu commented Dec 29, 2022

View reviewed changes

yxxhero approved these changes Jan 4, 2023

View reviewed changes

mumoshu merged commit 4688cf0 into main Jan 4, 2023

mumoshu deleted the go-back-to-gopkgin-yaml-v2-for-helmfile-v0.x branch January 4, 2023 09:17

github-actions Bot locked as resolved and limited conversation to collaborators Jan 6, 2023

Uh oh!

Uh oh!

Conversation

mumoshu commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

mumoshu commented Dec 29, 2022

Uh oh!

mumoshu commented Dec 29, 2022

Uh oh!

yxxhero commented Dec 29, 2022

Uh oh!

yxxhero commented Dec 29, 2022

Uh oh!

mumoshu commented Dec 29, 2022

Uh oh!

yxxhero commented Dec 29, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mumoshu commented Dec 29, 2022

Uh oh!

yxxhero commented Jan 1, 2023

Uh oh!

mumoshu commented Jan 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yxxhero commented Jan 4, 2023

Uh oh!

mumoshu commented Jan 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mumoshu commented Dec 28, 2022 •

edited

Loading

mumoshu commented Jan 2, 2023 •

edited

Loading