From 3e506eae02745fb550cb65f41dcecf918456bab1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mattias=20=C3=96hrn?= Date: Fri, 4 Jun 2021 11:06:22 +0200 Subject: [PATCH] PR feedback - more tests and some cleanup --- kyaml/kio/byteio_writer.go | 49 ++++++++++++++++---------- kyaml/kio/byteio_writer_test.go | 19 ++++++++++ kyaml/kio/filters/fmtr_test.go | 40 +++++++++++++++++++++ kyaml/kio/filters/testyaml/testyaml.go | 14 ++++++++ 4 files changed, 104 insertions(+), 18 deletions(-) diff --git a/kyaml/kio/byteio_writer.go b/kyaml/kio/byteio_writer.go index c22b2b262..a243ed911 100644 --- a/kyaml/kio/byteio_writer.go +++ b/kyaml/kio/byteio_writer.go @@ -12,7 +12,10 @@ import ( "sigs.k8s.io/kustomize/kyaml/yaml" ) -// ByteWriter writes ResourceNodes to bytes. +// ByteWriter writes ResourceNodes to bytes. Generally YAML encoding will be used but in the special +// case of writing a single, bare yaml.RNode that has a kioutil.PathAnnotation indicating that the +// target is a JSON file JSON encoding is used. See shouldJSONEncodeSingleBareNode below for more +// information. type ByteWriter struct { // Writer is where ResourceNodes are encoded. Writer io.Writer @@ -55,22 +58,8 @@ func (w ByteWriter) Write(nodes []*yaml.RNode) error { } } - // Check if the output is a single JSON file so we can force JSON encoding in that case. - // YAML flow style encoding may not be compatible because of unquoted strings and newlines - // introduced by the YAML marshaller in long string values. These newlines are insignificant - // when interpreted as YAML but invalid when interpreted as JSON. - jsonEncodeSingleNode := false - if w.WrappingKind == "" && len(nodes) == 1 { - if path, _, _ := kioutil.GetFileAnnotations(nodes[0]); path != "" { - filename := filepath.Base(path) - for _, glob := range JSONMatch { - if match, _ := filepath.Match(glob, filename); match { - jsonEncodeSingleNode = true - break - } - } - } - } + // Even though we use the this value further down we must check this before removing annotations + jsonEncodeSingleBareNode := w.shouldJSONEncodeSingleBareNode(nodes) for i := range nodes { // clean resources by removing annotations set by the Reader @@ -96,7 +85,7 @@ func (w ByteWriter) Write(nodes []*yaml.RNode) error { } } - if jsonEncodeSingleNode { + if jsonEncodeSingleBareNode { encoder := json.NewEncoder(w.Writer) encoder.SetIndent("", " ") return errors.Wrap(encoder.Encode(nodes[0])) @@ -145,3 +134,27 @@ func (w ByteWriter) Write(nodes []*yaml.RNode) error { yaml.UndoSerializationHacksOnNodes(nodes) return err } + +// shouldJSONEncodeSingleBareNode determines if nodes contain a single node that should not be +// wrapped and has a JSON file extension, which in turn means that the node should be JSON encoded. +// Note 1: this must be checked before any annotations to avoid losing information about the target +// filename extension. +// Note 2: JSON encoding should only be used for single, unwrapped nodes because multiple unwrapped +// nodes cannot be represented in JSON (no multi doc support). Furthermore, the typical use +// cases for wrapping nodes would likely not include later writing the whole wrapper to a +// .json file, i.e. there is no point risking any edge case information loss e.g. comments +// disappearing, that could come from JSON encoding the whole wrapper just to ensure that +// one (or all nodes) can be read as JSON. +func (w ByteWriter) shouldJSONEncodeSingleBareNode(nodes []*yaml.RNode) bool { + if w.WrappingKind == "" && len(nodes) == 1 { + if path, _, _ := kioutil.GetFileAnnotations(nodes[0]); path != "" { + filename := filepath.Base(path) + for _, glob := range JSONMatch { + if match, _ := filepath.Match(glob, filename); match { + return true + } + } + } + } + return false +} \ No newline at end of file diff --git a/kyaml/kio/byteio_writer_test.go b/kyaml/kio/byteio_writer_test.go index f8e3d47bd..c284d04a8 100644 --- a/kyaml/kio/byteio_writer_test.go +++ b/kyaml/kio/byteio_writer_test.go @@ -216,6 +216,25 @@ metadata: }`, }, + // + // Test Case + // + { + name: "encode_unformatted_valid_json", + items: []string{ + `{ "a": "b", metadata: { annotations: { config.kubernetes.io/path: test.json } } }`, + }, + + expectedOutput: `{ + "a": "b", + "metadata": { + "annotations": { + "config.kubernetes.io/path": "test.json" + } + } +}`, + }, + // // Test Case // diff --git a/kyaml/kio/filters/fmtr_test.go b/kyaml/kio/filters/fmtr_test.go index cebb2da60..e988a3ff5 100644 --- a/kyaml/kio/filters/fmtr_test.go +++ b/kyaml/kio/filters/fmtr_test.go @@ -743,6 +743,46 @@ func TestFormatFileOrDirectory_ymlExtFile(t *testing.T) { assert.Equal(t, string(testyaml.FormattedYaml1), string(b)) } +// TestFormatFileOrDirectory_YamlExtFileWithJson verifies that the JSON compatible flow style +// YAML content is formatted as such. +func TestFormatFileOrDirectory_YamlExtFileWithJson(t *testing.T) { + // write the unformatted JSON file contents + f, err := ioutil.TempFile("", "yamlfmt*.yaml") + assert.NoError(t, err) + defer os.Remove(f.Name()) + err = ioutil.WriteFile(f.Name(), testyaml.UnformattedJSON1, 0600) + assert.NoError(t, err) + + // format the file + err = FormatFileOrDirectory(f.Name()) + assert.NoError(t, err) + + // check the result is formatted as yaml + b, err := ioutil.ReadFile(f.Name()) + assert.NoError(t, err) + assert.Equal(t, string(testyaml.FormattedFlowYAML1), string(b)) +} + +// TestFormatFileOrDirectory_JsonExtFileWithNotModified verifies that a file with .json extensions +// and JSON contents won't be modified. +func TestFormatFileOrDirectory_JsonExtFileWithNotModified(t *testing.T) { + // write the unformatted JSON file contents + f, err := ioutil.TempFile("", "yamlfmt*.json") + assert.NoError(t, err) + defer os.Remove(f.Name()) + err = ioutil.WriteFile(f.Name(), testyaml.UnformattedJSON1, 0600) + assert.NoError(t, err) + + // format the file + err = FormatFileOrDirectory(f.Name()) + assert.NoError(t, err) + + // check the result is formatted as yaml + b, err := ioutil.ReadFile(f.Name()) + assert.NoError(t, err) + assert.Equal(t, string(testyaml.UnformattedJSON1), string(b)) +} + // TestFormatFileOrDirectory_partialKubernetesYamlFile verifies that if a yaml file contains both // Kubernetes and non-Kubernetes documents, it will only format the Kubernetes documents func TestFormatFileOrDirectory_partialKubernetesYamlFile(t *testing.T) { diff --git a/kyaml/kio/filters/testyaml/testyaml.go b/kyaml/kio/filters/testyaml/testyaml.go index c54d8d305..01644b311 100644 --- a/kyaml/kio/filters/testyaml/testyaml.go +++ b/kyaml/kio/filters/testyaml/testyaml.go @@ -68,6 +68,15 @@ metadata: selfLink: "" `) +var UnformattedJSON1 = []byte(` +{ + "spec": "a", + "status": {"conditions": [3, 1, 2]}, + "apiVersion": "example.com/v1beta1", + "kind": "MyType" +} +`) + var FormattedYaml1 = []byte(`apiVersion: example.com/v1beta1 kind: MyType spec: a @@ -88,6 +97,11 @@ status2: - 2 `) +var FormattedFlowYAML1 = []byte( + `{"apiVersion": "example.com/v1beta1", "kind": "MyType", "spec": "a", "status": {"conditions": [ + 3, 1, 2]}} +`) + var FormattedYaml3 = []byte(`apiVersion: v1 kind: List metadata: