diff --git a/kyaml/fieldmeta/fieldmeta.go b/kyaml/fieldmeta/fieldmeta.go index b4d6383d4..6006f4466 100644 --- a/kyaml/fieldmeta/fieldmeta.go +++ b/kyaml/fieldmeta/fieldmeta.go @@ -5,6 +5,7 @@ package fieldmeta import ( "encoding/json" + "reflect" "strconv" "strings" @@ -36,16 +37,29 @@ type PartialFieldSetter struct { Value string `yaml:"value" json:"value"` } +// IsEmpty returns true if the FieldMeta has any empty Schema +func (fm *FieldMeta) IsEmpty() bool { + if fm == nil { + return true + } + return reflect.DeepEqual(fm.Schema, spec.Schema{}) +} + // Read reads the FieldMeta from a node func (fm *FieldMeta) Read(n *yaml.RNode) error { - if n.YNode().LineComment != "" { - v := strings.TrimLeft(n.YNode().LineComment, "#") + // check for metadata on head and line comments + comments := []string{n.YNode().LineComment, n.YNode().HeadComment} + for _, c := range comments { + if c == "" { + continue + } + c := strings.TrimLeft(c, "#") // if it doesn't Unmarshal that is fine, it means there is no metadata // other comments are valid, they just don't parse - // TODO: consider most sophisticated parsing techniques similar to what is used + // TODO: consider more sophisticated parsing techniques similar to what is used // for go struct tags. - if err := fm.Schema.UnmarshalJSON([]byte(v)); err != nil { + if err := fm.Schema.UnmarshalJSON([]byte(c)); err != nil { // note: don't return an error if the comment isn't a fieldmeta struct return nil } diff --git a/kyaml/openapi/openapi.go b/kyaml/openapi/openapi.go index a349388fa..11c9d2422 100644 --- a/kyaml/openapi/openapi.go +++ b/kyaml/openapi/openapi.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "io/ioutil" + "reflect" "sync" "github.com/go-openapi/spec" @@ -32,6 +33,14 @@ type ResourceSchema struct { Schema *spec.Schema } +// IsEmpty returns true if the ResourceSchema is empty +func (rs *ResourceSchema) IsEmpty() bool { + if rs == nil || rs.Schema == nil { + return true + } + return reflect.DeepEqual(*rs.Schema, spec.Schema{}) +} + // SchemaForResourceType returns the Schema for the given Resource // TODO(pwittrock): create a version of this function that will return a schema // which can be used for duck-typed Resources -- e.g. contains common fields such @@ -193,15 +202,15 @@ func SuppressBuiltInSchemaUse() { } // Elements returns the Schema for the elements of an array. -func (r *ResourceSchema) Elements() *ResourceSchema { +func (rs *ResourceSchema) Elements() *ResourceSchema { // load the schema from swagger.json initSchema() - if len(r.Schema.Type) != 1 || r.Schema.Type[0] != "array" { + if len(rs.Schema.Type) != 1 || rs.Schema.Type[0] != "array" { // either not an array, or array has multiple types return nil } - s := *r.Schema.Items.Schema + s := *rs.Schema.Items.Schema for s.Ref.String() != "" { sc, e := Resolve(&s.Ref) if e != nil { @@ -219,8 +228,8 @@ const Elements = "[]" // Field is called. // If any Field or Elements call returns nil, then Lookup returns // nil immediately. -func (r *ResourceSchema) Lookup(path ...string) *ResourceSchema { - s := r +func (rs *ResourceSchema) Lookup(path ...string) *ResourceSchema { + s := rs for _, p := range path { if s == nil { break @@ -235,19 +244,19 @@ func (r *ResourceSchema) Lookup(path ...string) *ResourceSchema { } // Field returns the Schema for a field. -func (r *ResourceSchema) Field(field string) *ResourceSchema { +func (rs *ResourceSchema) Field(field string) *ResourceSchema { // load the schema from swagger.json initSchema() // locate the Schema - s, found := r.Schema.Properties[field] + s, found := rs.Schema.Properties[field] switch { case found: // no-op, continue with s as the schema - case r.Schema.AdditionalProperties != nil && r.Schema.AdditionalProperties.Schema != nil: + case rs.Schema.AdditionalProperties != nil && rs.Schema.AdditionalProperties.Schema != nil: // map field type -- use Schema of the value // (the key doesn't matter, they all have the same value type) - s = *r.Schema.AdditionalProperties.Schema + s = *rs.Schema.AdditionalProperties.Schema default: // no Schema found from either swagger.json or line comments return nil @@ -267,14 +276,14 @@ func (r *ResourceSchema) Field(field string) *ResourceSchema { } // PatchStrategyAndKey returns the patch strategy and merge key extensions -func (r *ResourceSchema) PatchStrategyAndKey() (string, string) { - ps, found := r.Schema.Extensions[kubernetesPatchStrategyExtensionKey] +func (rs *ResourceSchema) PatchStrategyAndKey() (string, string) { + ps, found := rs.Schema.Extensions[kubernetesPatchStrategyExtensionKey] if !found { // merge key and patch strategy must appear together return "", "" } - mk, found := r.Schema.Extensions[kubernetesMergeKeyExtensionKey] + mk, found := rs.Schema.Extensions[kubernetesMergeKeyExtensionKey] if !found { // merge key and patch strategy must appear together return "", "" diff --git a/kyaml/yaml/merge2/element_test.go b/kyaml/yaml/merge2/element_test.go index 2a9cd5d6e..7338ee6a4 100644 --- a/kyaml/yaml/merge2/element_test.go +++ b/kyaml/yaml/merge2/element_test.go @@ -4,21 +4,21 @@ package merge2_test var elementTestCases = []testCase{ - {`merge Element -- keep field in dest`, - ` + {description: `merge Element -- keep field in dest`, + source: ` kind: Deployment items: - name: foo image: foo:v1 `, - ` + dest: ` kind: Deployment items: - name: foo image: foo:v0 command: ['run.sh'] `, - ` + expected: ` kind: Deployment items: - name: foo @@ -27,21 +27,21 @@ items: `, }, - {`merge Element -- add field to dest`, - ` + {description: `merge Element -- add field to dest`, + source: ` kind: Deployment items: - name: foo image: foo:v1 command: ['run.sh'] `, - ` + dest: ` kind: Deployment items: - name: foo image: foo:v0 `, - ` + expected: ` kind: Deployment items: - name: foo @@ -50,19 +50,19 @@ items: `, }, - {`merge Element -- add list, empty in dest`, - ` + {description: `merge Element -- add list, empty in dest`, + source: ` kind: Deployment items: - name: foo image: foo:v1 command: ['run.sh'] `, - ` + dest: ` kind: Deployment items: [] `, - ` + expected: ` kind: Deployment items: - name: foo @@ -71,18 +71,18 @@ items: `, }, - {`merge Element -- add list, missing from dest`, - ` + {description: `merge Element -- add list, missing from dest`, + source: ` kind: Deployment items: - name: foo image: foo:v1 command: ['run.sh'] `, - ` + dest: ` kind: Deployment `, - ` + expected: ` kind: Deployment items: - name: foo @@ -91,8 +91,8 @@ items: `, }, - {`merge Element -- add Element first`, - ` + {description: `merge Element -- add Element first`, + source: ` kind: Deployment items: - name: bar @@ -101,13 +101,13 @@ items: - name: foo image: foo:v1 `, - ` + dest: ` kind: Deployment items: - name: foo image: foo:v0 `, - ` + expected: ` kind: Deployment items: - name: foo @@ -118,8 +118,8 @@ items: `, }, - {`merge Element -- add Element second`, - ` + {description: `merge Element -- add Element second`, + source: ` kind: Deployment items: - name: foo @@ -128,13 +128,13 @@ items: image: bar:v1 command: ['run2.sh'] `, - ` + dest: ` kind: Deployment items: - name: foo image: foo:v0 `, - ` + expected: ` kind: Deployment items: - name: foo @@ -148,11 +148,11 @@ items: // // Test Case // - {`keep list -- list missing from src`, - ` + {description: `keep list -- list missing from src`, + source: ` kind: Deployment `, - ` + dest: ` kind: Deployment items: - name: foo @@ -161,7 +161,7 @@ items: image: bar:v1 command: ['run2.sh'] `, - ` + expected: ` kind: Deployment items: - name: foo @@ -175,14 +175,14 @@ items: // // Test Case // - {`keep Element -- element missing in src`, - ` + {description: `keep Element -- element missing in src`, + source: ` kind: Deployment items: - name: foo image: foo:v1 `, - ` + dest: ` kind: Deployment items: - name: foo @@ -191,7 +191,7 @@ items: image: bar:v1 command: ['run2.sh'] `, - ` + expected: ` kind: Deployment items: - name: foo @@ -205,12 +205,12 @@ items: // // Test Case // - {`keep element -- empty list in src`, - ` + {description: `keep element -- empty list in src`, + source: ` kind: Deployment items: {} `, - ` + dest: ` kind: Deployment items: - name: foo @@ -219,7 +219,7 @@ items: image: bar:v1 command: ['run2.sh'] `, - ` + expected: ` kind: Deployment items: - name: foo @@ -233,12 +233,12 @@ items: // // Test Case // - {`remove Element -- null in src`, - ` + {description: `remove Element -- null in src`, + source: ` kind: Deployment items: null `, - ` + dest: ` kind: Deployment items: - name: foo @@ -247,8 +247,97 @@ items: image: bar:v1 command: ['run2.sh'] `, - ` + expected: ` kind: Deployment `, }, + + // + // Test Case + // + {description: `no infer merge keys no merge'`, + source: ` +kind: Deployment +containers: +- name: foo + command: ['run2.sh'] +`, + dest: ` +kind: Deployment +containers: +- name: foo + image: foo:bar +`, + expected: ` +kind: Deployment +containers: +- name: foo + command: ['run2.sh'] +`, + noInfer: true, + }, + + // + // Test Case + // + {description: `no infer merge keys merge using schema`, + source: ` +kind: Deployment +apiVersion: apps/v1 +spec: + template: + spec: + containers: + - name: foo + command: ['run2.sh'] +`, + dest: ` +kind: Deployment +apiVersion: apps/v1 +spec: + template: + spec: + containers: + - name: foo + image: foo:bar +`, + expected: ` +kind: Deployment +apiVersion: apps/v1 +spec: + template: + spec: + containers: + - name: foo + image: foo:bar + command: ['run2.sh'] +`, + noInfer: true, + }, + + // + // Test Case + // + {description: `no infer merge keys merge using explicit schema as line comment'`, + source: ` +kind: Deployment +containers: +- name: foo + command: ['run2.sh'] +`, + dest: ` +kind: Deployment +containers: # {"items":{"$ref": "#/definitions/io.k8s.api.core.v1.Container"},"type":"array","x-kubernetes-patch-merge-key":"name","x-kubernetes-patch-strategy": "merge"} +- name: foo # hell ow + image: foo:bar +`, + expected: ` +kind: Deployment +containers: # {"items":{"$ref": "#/definitions/io.k8s.api.core.v1.Container"},"type":"array","x-kubernetes-patch-merge-key":"name","x-kubernetes-patch-strategy": "merge"} +- name: foo + image: foo:bar + command: ['run2.sh'] +`, + noInfer: true, + }, } diff --git a/kyaml/yaml/merge2/list_test.go b/kyaml/yaml/merge2/list_test.go index 0aa6a3dd5..433524c60 100644 --- a/kyaml/yaml/merge2/list_test.go +++ b/kyaml/yaml/merge2/list_test.go @@ -4,21 +4,21 @@ package merge2_test var listTestCases = []testCase{ - {`replace List -- different value in dest`, - ` + {description: `replace List -- different value in dest`, + source: ` kind: Deployment items: - 1 - 2 - 3 `, - ` + dest: ` kind: Deployment items: - 0 - 1 `, - ` + expected: ` kind: Deployment items: - 1 @@ -27,18 +27,18 @@ items: `, }, - {`replace List -- missing from dest`, - ` + {description: `replace List -- missing from dest`, + source: ` kind: Deployment items: - 1 - 2 - 3 `, - ` + dest: ` kind: Deployment `, - ` + expected: ` kind: Deployment items: - 1 @@ -50,22 +50,22 @@ items: // // Test Case // - {`keep List -- same value in src and dest`, - ` + {description: `keep List -- same value in src and dest`, + source: ` kind: Deployment items: - 1 - 2 - 3 `, - ` + dest: ` kind: Deployment items: - 1 - 2 - 3 `, - ` + expected: ` kind: Deployment items: - 1 @@ -77,18 +77,18 @@ items: // // Test Case // - {`keep List -- unspecified in src`, - ` + {description: `keep List -- unspecified in src`, + source: ` kind: Deployment `, - ` + dest: ` kind: Deployment items: - 1 - 2 - 3 `, - ` + expected: ` kind: Deployment items: - 1 @@ -100,19 +100,19 @@ items: // // Test Case // - {`remove List -- null in src`, - ` + {description: `remove List -- null in src`, + source: ` kind: Deployment items: null `, - ` + dest: ` kind: Deployment items: - 1 - 2 - 3 `, - ` + expected: ` kind: Deployment `, }, @@ -120,19 +120,19 @@ kind: Deployment // // Test Case // - {`remove list -- empty in src`, - ` + {description: `remove list -- empty in src`, + source: ` kind: Deployment items: {} `, - ` + dest: ` kind: Deployment items: - 1 - 2 - 3 `, - ` + expected: ` kind: Deployment items: {} `, diff --git a/kyaml/yaml/merge2/map_test.go b/kyaml/yaml/merge2/map_test.go index cfdbee357..405da742e 100644 --- a/kyaml/yaml/merge2/map_test.go +++ b/kyaml/yaml/merge2/map_test.go @@ -4,19 +4,19 @@ package merge2_test var mapTestCases = []testCase{ - {`merge Map -- update field in dest`, - ` + {description: `merge Map -- update field in dest`, + source: ` kind: Deployment spec: foo: bar1 `, - ` + dest: ` kind: Deployment spec: foo: bar0 baz: buz `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -24,19 +24,19 @@ spec: `, }, - {`merge Map -- add field to dest`, - ` + {description: `merge Map -- add field to dest`, + source: ` kind: Deployment spec: foo: bar1 baz: buz `, - ` + dest: ` kind: Deployment spec: foo: bar0 `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -44,18 +44,18 @@ spec: `, }, - {`merge Map -- add list, empty in dest`, - ` + {description: `merge Map -- add list, empty in dest`, + source: ` kind: Deployment spec: foo: bar1 baz: buz `, - ` + dest: ` kind: Deployment spec: {} `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -63,17 +63,17 @@ spec: `, }, - {`merge Map -- add list, missing from dest`, - ` + {description: `merge Map -- add list, missing from dest`, + source: ` kind: Deployment spec: foo: bar1 baz: buz `, - ` + dest: ` kind: Deployment `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -81,19 +81,19 @@ spec: `, }, - {`merge Map -- add Map first`, - ` + {description: `merge Map -- add Map first`, + source: ` kind: Deployment spec: foo: bar1 baz: buz `, - ` + dest: ` kind: Deployment spec: foo: bar1 `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -101,19 +101,19 @@ spec: `, }, - {`merge Map -- add Map second`, - ` + {description: `merge Map -- add Map second`, + source: ` kind: Deployment spec: baz: buz foo: bar1 `, - ` + dest: ` kind: Deployment spec: foo: bar1 `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -124,17 +124,17 @@ spec: // // Test Case // - {`keep map -- map missing from src`, - ` + {description: `keep map -- map missing from src`, + source: ` kind: Deployment `, - ` + dest: ` kind: Deployment spec: foo: bar1 baz: buz `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -145,18 +145,18 @@ spec: // // Test Case // - {`keep map -- empty list in src`, - ` + {description: `keep map -- empty list in src`, + source: ` kind: Deployment items: {} `, - ` + dest: ` kind: Deployment spec: foo: bar1 baz: buz `, - ` + expected: ` kind: Deployment spec: foo: bar1 @@ -168,18 +168,18 @@ items: {} // // Test Case // - {`remove Map -- null in src`, - ` + {description: `remove Map -- null in src`, + source: ` kind: Deployment spec: null `, - ` + dest: ` kind: Deployment spec: foo: bar1 baz: buz `, - ` + expected: ` kind: Deployment `, }, diff --git a/kyaml/yaml/merge2/merge2.go b/kyaml/yaml/merge2/merge2.go index 86f3b5ece..973d3e2e9 100644 --- a/kyaml/yaml/merge2/merge2.go +++ b/kyaml/yaml/merge2/merge2.go @@ -6,6 +6,7 @@ package merge2 import ( + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" "sigs.k8s.io/kustomize/kyaml/yaml/walk" ) @@ -16,7 +17,7 @@ func Merge(src, dest *yaml.RNode) (*yaml.RNode, error) { } // Merge parses the arguments, and merges fields from srcStr into destStr. -func MergeStrings(srcStr, destStr string) (string, error) { +func MergeStrings(srcStr, destStr string, infer bool) (string, error) { src, err := yaml.Parse(srcStr) if err != nil { return "", err @@ -26,10 +27,15 @@ func MergeStrings(srcStr, destStr string) (string, error) { return "", err } - result, err := Merge(src, dest) + result, err := walk.Walker{ + Sources: []*yaml.RNode{dest, src}, + Visitor: Merger{}, + InferAssociativeLists: infer, + }.Walk() if err != nil { return "", err } + return result.String() } @@ -39,7 +45,7 @@ type Merger struct { var _ walk.Visitor = Merger{} -func (m Merger) VisitMap(nodes walk.Sources) (*yaml.RNode, error) { +func (m Merger) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { if err := m.SetComments(nodes); err != nil { return nil, err } @@ -58,7 +64,7 @@ func (m Merger) VisitMap(nodes walk.Sources) (*yaml.RNode, error) { return nodes.Dest(), nil } -func (m Merger) VisitScalar(nodes walk.Sources) (*yaml.RNode, error) { +func (m Merger) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { if err := m.SetComments(nodes); err != nil { return nil, err } @@ -73,7 +79,7 @@ func (m Merger) VisitScalar(nodes walk.Sources) (*yaml.RNode, error) { return nodes.Dest(), nil } -func (m Merger) VisitList(nodes walk.Sources, kind walk.ListKind) (*yaml.RNode, error) { +func (m Merger) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, kind walk.ListKind) (*yaml.RNode, error) { if err := m.SetComments(nodes); err != nil { return nil, err } diff --git a/kyaml/yaml/merge2/merge2_old_test.go b/kyaml/yaml/merge2/merge2_old_test.go index e4855b72c..a05d00b1c 100644 --- a/kyaml/yaml/merge2/merge2_old_test.go +++ b/kyaml/yaml/merge2/merge2_old_test.go @@ -365,7 +365,7 @@ a: b: # header comment c: d -`) +`, true) if !assert.NoError(t, err) { return } @@ -385,7 +385,7 @@ a: b: c: d # footer comment -`) +`, true) if !assert.NoError(t, err) { return } @@ -404,7 +404,7 @@ a: a: b: c: d # line comment -`) +`, true) if !assert.NoError(t, err) { return } @@ -426,7 +426,7 @@ a: b: # replace comment c: d -`) +`, true) if !assert.NoError(t, err) { return } @@ -447,7 +447,7 @@ a: b: c: d # replace comment -`) +`, true) if !assert.NoError(t, err) { return } @@ -466,7 +466,7 @@ a: a: b: c: d # replace comment -`) +`, true) if !assert.NoError(t, err) { return } @@ -484,7 +484,7 @@ a: a: b: c: d # replace comment -`) +`, true) if !assert.NoError(t, err) { return } diff --git a/kyaml/yaml/merge2/merge2_test.go b/kyaml/yaml/merge2/merge2_test.go index 54ad164ae..6a3e846bf 100644 --- a/kyaml/yaml/merge2/merge2_test.go +++ b/kyaml/yaml/merge2/merge2_test.go @@ -17,24 +17,27 @@ var testCases = [][]testCase{scalarTestCases, listTestCases, elementTestCases, m func TestMerge(t *testing.T) { for i := range testCases { - for _, tc := range testCases[i] { - actual, err := MergeStrings(tc.source, tc.dest) - if !assert.NoError(t, err, tc.description) { - t.FailNow() - } - e, err := filters.FormatInput(bytes.NewBufferString(tc.expected)) - if !assert.NoError(t, err) { - t.FailNow() - } - estr := strings.TrimSpace(e.String()) - a, err := filters.FormatInput(bytes.NewBufferString(actual)) - if !assert.NoError(t, err) { - t.FailNow() - } - astr := strings.TrimSpace(a.String()) - if !assert.Equal(t, estr, astr, tc.description) { - t.FailNow() - } + for j := range testCases[i] { + tc := testCases[i][j] + t.Run(tc.description, func(t *testing.T) { + actual, err := MergeStrings(tc.source, tc.dest, !tc.noInfer) + if !assert.NoError(t, err, tc.description) { + t.FailNow() + } + e, err := filters.FormatInput(bytes.NewBufferString(tc.expected)) + if !assert.NoError(t, err) { + t.FailNow() + } + estr := strings.TrimSpace(e.String()) + a, err := filters.FormatInput(bytes.NewBufferString(actual)) + if !assert.NoError(t, err) { + t.FailNow() + } + astr := strings.TrimSpace(a.String()) + if !assert.Equal(t, estr, astr, tc.description) { + t.FailNow() + } + }) } } } @@ -44,4 +47,5 @@ type testCase struct { source string dest string expected string + noInfer bool } diff --git a/kyaml/yaml/merge2/scalar_test.go b/kyaml/yaml/merge2/scalar_test.go index 2cc7947e8..029a55683 100644 --- a/kyaml/yaml/merge2/scalar_test.go +++ b/kyaml/yaml/merge2/scalar_test.go @@ -4,30 +4,30 @@ package merge2_test var scalarTestCases = []testCase{ - {`replace scalar -- different value in dest`, - ` + {description: `replace scalar -- different value in dest`, + source: ` kind: Deployment field: value1 `, - ` + dest: ` kind: Deployment field: value0 `, - ` + expected: ` kind: Deployment field: value1 `, }, - {`replace scalar -- missing from dest`, - ` + {description: `replace scalar -- missing from dest`, + source: ` kind: Deployment field: value1 `, - ` + dest: ` kind: Deployment `, - ` + expected: ` kind: Deployment field: value1 `, @@ -36,16 +36,16 @@ field: value1 // // Test Case // - {`keep scalar -- same value in src and dest`, - ` + {description: `keep scalar -- same value in src and dest`, + source: ` kind: Deployment field: value1 `, - ` + dest: ` kind: Deployment field: value1 `, - ` + expected: ` kind: Deployment field: value1 `, @@ -54,15 +54,15 @@ field: value1 // // Test Case // - {`keep scalar -- unspecified in src`, - ` + {description: `keep scalar -- unspecified in src`, + source: ` kind: Deployment `, - ` + dest: ` kind: Deployment field: value1 `, - ` + expected: ` kind: Deployment field: value1 `, @@ -71,16 +71,16 @@ field: value1 // // Test Case // - {`remove scalar -- null in src`, - ` + {description: `remove scalar -- null in src`, + source: ` kind: Deployment field: null `, - ` + dest: ` kind: Deployment field: value1 `, - ` + expected: ` kind: Deployment `, }, @@ -88,16 +88,16 @@ kind: Deployment // // Test Case // - {`remove scalar -- empty in src`, - ` + {description: `remove scalar -- empty in src`, + source: ` kind: Deployment field: {} `, - ` + dest: ` kind: Deployment field: value1 `, - ` + expected: ` kind: Deployment field: {} `, @@ -106,15 +106,15 @@ field: {} // // Test Case // - {`remove scalar -- null in src, missing in dest`, - ` + {description: `remove scalar -- null in src, missing in dest`, + source: ` kind: Deployment field: null `, - ` + dest: ` kind: Deployment `, - ` + expected: ` kind: Deployment `, }, @@ -122,15 +122,15 @@ kind: Deployment // // Test Case // - {`merge an empty value`, - ` + {description: `merge an empty value`, + source: ` kind: Deployment field: {} `, - ` + dest: ` kind: Deployment `, - ` + expected: ` kind: Deployment field: {} `, diff --git a/kyaml/yaml/merge3/element_test.go b/kyaml/yaml/merge3/element_test.go index 7438d5ce8..9e7cf3fd7 100644 --- a/kyaml/yaml/merge3/element_test.go +++ b/kyaml/yaml/merge3/element_test.go @@ -7,14 +7,14 @@ var elementTestCases = []testCase{ // // Test Case // - {`Add an element to an existing list`, - ` + {description: `Add an element to an existing list`, + origin: ` kind: Deployment containers: - name: foo image: foo:1 `, - ` + update: ` kind: Deployment containers: - name: foo @@ -22,13 +22,13 @@ containers: - name: baz image: baz:2 `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:1 `, - ` + expected: ` kind: Deployment containers: - name: foo @@ -36,65 +36,65 @@ containers: - image: baz:2 name: baz -`, nil}, +`}, // // Test Case // - {`Add an element to a non-existing list`, - ` + {description: `Add an element to a non-existing list`, + origin: ` kind: Deployment`, - ` + update: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + local: ` kind: Deployment `, - ` + expected: ` kind: Deployment containers: - image: foo:bar name: foo -`, nil}, +`}, - {`Add an element to a non-existing list, existing in dest`, - ` + {description: `Add an element to a non-existing list, existing in dest`, + origin: ` kind: Deployment`, - ` + update: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + local: ` kind: Deployment containers: - name: baz image: baz:bar `, - ` + expected: ` kind: Deployment containers: - name: baz image: baz:bar - image: foo:bar name: foo -`, nil}, +`}, // // Test Case // TODO(pwittrock): Figure out if there is something better we can do here // This element is missing from the destination -- only the new fields are added - {`Add a field to the element, element missing from dest`, - ` + {description: `Add a field to the element, element missing from dest`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar`, - ` + update: ` kind: Deployment containers: - name: foo @@ -102,22 +102,22 @@ containers: command: - run.sh `, - ` + local: ` kind: Deployment `, - ` + expected: ` kind: Deployment containers: - command: - run.sh name: foo -`, nil}, +`}, // // Test Case // - {`Update a field on the elem, element missing from the dest`, - ` + {description: `Update a field on the elem, element missing from the dest`, + origin: ` kind: Deployment containers: - name: foo @@ -125,7 +125,7 @@ containers: command: - run.sh `, - ` + update: ` kind: Deployment containers: - name: foo @@ -133,210 +133,210 @@ containers: command: - run2.sh `, - ` + local: ` kind: Deployment `, - ` + expected: ` kind: Deployment containers: - command: - run2.sh name: foo -`, nil}, +`}, // // Test Case // - {`Update a field on the elem, element present in the dest`, - ` + {description: `Update a field on the elem, element present in the dest`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run.sh'] `, - ` + update: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run2.sh'] `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run.sh'] `, - ` + expected: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run2.sh'] -`, nil}, +`}, // // Test Case // - {`Add a field on the elem, element present in the dest`, - ` + {description: `Add a field on the elem, element present in the dest`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + update: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run2.sh'] `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + expected: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run2.sh'] -`, nil}, +`}, // // Test Case // - {`Add a field on the elem, element and field present in the dest`, - ` + {description: `Add a field on the elem, element and field present in the dest`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + update: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run2.sh'] `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run.sh'] `, - ` + expected: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run2.sh'] -`, nil}, +`}, // // Test Case // - {`Ignore an element`, - ` + {description: `Ignore an element`, + origin: ` kind: Deployment containers: {} `, - ` + update: ` kind: Deployment containers: {} `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + expected: ` kind: Deployment containers: - name: foo image: foo:bar -`, nil}, +`}, // // Test Case // - {`Leave deleted`, - ` + {description: `Leave deleted`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + update: ` kind: Deployment `, - ` + local: ` kind: Deployment `, - ` + expected: ` kind: Deployment -`, nil}, +`}, // // Test Case // - {`Remove an element -- matching`, - ` + {description: `Remove an element -- matching`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + update: ` kind: Deployment `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + expected: ` kind: Deployment -`, nil}, +`}, // // Test Case // - {`Remove an element -- field missing from update`, - ` + {description: `Remove an element -- field missing from update`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + update: ` kind: Deployment `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run.sh'] `, - ` + expected: ` kind: Deployment -`, nil}, +`}, // // Test Case // - {`Remove an element -- element missing`, - ` + {description: `Remove an element -- element missing`, + origin: ` kind: Deployment containers: - name: foo @@ -344,13 +344,13 @@ containers: - name: baz image: baz:bar `, - ` + update: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + local: ` kind: Deployment containers: - name: foo @@ -359,60 +359,273 @@ containers: - name: baz image: baz:bar `, - ` + expected: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run.sh'] -`, nil}, +`}, // // Test Case // - {`Remove an element -- empty containers`, - ` + {description: `Remove an element -- empty containers`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + update: ` kind: Deployment containers: {} `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run.sh'] `, - ` + expected: ` kind: Deployment -`, nil}, +`}, // // Test Case // - {`Remove an element -- missing list field`, - ` + {description: `Remove an element -- missing list field`, + origin: ` kind: Deployment containers: - name: foo image: foo:bar `, - ` + update: ` kind: Deployment `, - ` + local: ` kind: Deployment containers: - name: foo image: foo:bar command: ['run.sh'] `, - ` + expected: ` kind: Deployment -`, nil}, +`}, + + // + // Test Case + // + {description: `no infer merge keys no merge'`, + origin: ` +kind: Deployment +containers: +- name: foo +`, + update: ` +kind: Deployment +containers: +- name: foo + command: ['run2.sh'] +`, + local: ` +kind: Deployment +containers: +- name: foo + image: foo:bar +`, + expected: ` +kind: Deployment +containers: +- name: foo + command: ['run2.sh'] +`, + noInfer: true, + }, + + // + // Test Case + // + {description: `no infer merge keys merge using schema`, + origin: ` +kind: Deployment +apiVersion: apps/v1 +spec: + template: + spec: + containers: + - name: foo +`, + update: ` +kind: Deployment +apiVersion: apps/v1 +spec: + template: + spec: + containers: + - name: foo + command: ['run2.sh'] +`, + local: ` +kind: Deployment +apiVersion: apps/v1 +spec: + template: + spec: + containers: + - name: foo + image: foo:bar +`, + expected: ` +kind: Deployment +apiVersion: apps/v1 +spec: + template: + spec: + containers: + - name: foo + image: foo:bar + command: ['run2.sh'] +`, + noInfer: true, + }, + + // + // Test Case + // + {description: `no infer merge keys merge using explicit schema as line comment'`, + origin: ` +kind: Deployment +containers: +- name: foo +`, + update: ` +kind: Deployment +containers: +- name: foo + command: ['run2.sh'] +`, + local: ` +kind: Deployment +containers: # {"items":{"$ref": "#/definitions/io.k8s.api.core.v1.Container"},"type":"array","x-kubernetes-patch-merge-key":"name","x-kubernetes-patch-strategy": "merge"} +- name: foo # hell ow + image: foo:bar +`, + expected: ` +kind: Deployment +containers: # {"items":{"$ref": "#/definitions/io.k8s.api.core.v1.Container"},"type":"array","x-kubernetes-patch-merge-key":"name","x-kubernetes-patch-strategy": "merge"} +- name: foo # hell ow + image: foo:bar + command: ['run2.sh'] +`, + noInfer: true, + }, + + // + // Test Case + // + {description: `no infer merge keys merge using explicit schema as head comment'`, + origin: ` +kind: Deployment +containers: +- name: foo +`, + update: ` +kind: Deployment +containers: +- name: foo + command: ['run2.sh'] +`, + local: ` +kind: Deployment +# {"items":{"$ref": "#/definitions/io.k8s.api.core.v1.Container"},"type":"array","x-kubernetes-patch-merge-key":"name","x-kubernetes-patch-strategy": "merge"} +containers: +- name: foo # hell ow + image: foo:bar +`, + expected: ` +kind: Deployment +# {"items":{"$ref": "#/definitions/io.k8s.api.core.v1.Container"},"type":"array","x-kubernetes-patch-merge-key":"name","x-kubernetes-patch-strategy": "merge"} +containers: +- name: foo # hell ow + image: foo:bar + command: ['run2.sh'] +`, + noInfer: true, + }, + + // + // Test Case + // + {description: `no infer merge keys merge using explicit schema to parent field'`, + origin: ` +kind: Deployment +spec: + containers: + - name: foo +`, + update: ` +kind: Deployment +spec: + containers: + - name: foo + command: ['run2.sh'] +`, + local: ` +kind: Deployment +spec: # {"$ref":"#/definitions/io.k8s.api.core.v1.PodSpec"} + containers: + - name: foo # hell ow + image: foo:bar +`, + expected: ` +kind: Deployment +spec: # {"$ref":"#/definitions/io.k8s.api.core.v1.PodSpec"} + containers: + - name: foo # hell ow + image: foo:bar + command: ['run2.sh'] +`, + noInfer: true, + }, + + // + // Test Case + // + {description: `no infer merge keys merge using explicit schema to parent field header'`, + origin: ` +kind: Deployment +spec: + containers: + - name: foo +`, + update: ` +kind: Deployment +spec: + containers: + - name: foo + command: ['run2.sh'] +`, + local: ` +kind: Deployment +# {"$ref":"#/definitions/io.k8s.api.core.v1.PodSpec"} +spec: + containers: + - name: foo # hell ow + image: foo:bar +`, + expected: ` +kind: Deployment +# {"$ref":"#/definitions/io.k8s.api.core.v1.PodSpec"} +spec: + containers: + - name: foo # hell ow + image: foo:bar + command: ['run2.sh'] +`, + noInfer: true, + }, } diff --git a/kyaml/yaml/merge3/list_test.go b/kyaml/yaml/merge3/list_test.go index 0574a5687..467c9e5b9 100644 --- a/kyaml/yaml/merge3/list_test.go +++ b/kyaml/yaml/merge3/list_test.go @@ -9,224 +9,226 @@ var listTestCases = []testCase{ // // Test Case // - {`Replace list`, - ` + {description: `Replace list`, + origin: ` list: - 1 - 2 - 3`, - ` + update: ` list: - 2 - 3 - 4`, - ` + local: ` list: - 1 - 2 - 3`, - ` + expected: ` list: - 2 - 3 -- 4`, nil}, +- 4`}, // // Test Case // - {`Add an updated list`, - ` + {description: `Add an updated list`, + origin: ` apiVersion: apps/v1 list: # old value - 1 - 2 - 3 `, - ` + update: ` apiVersion: apps/v1 list: # new value - 2 - 3 - 4 `, - ` + local: ` apiVersion: apps/v1`, - ` + expected: ` apiVersion: apps/v1 list: - 2 - 3 - 4 -`, nil}, +`}, // // Test Case // - {`Add keep an omitted field`, - ` + {description: `Add keep an omitted field`, + origin: ` apiVersion: apps/v1 kind: Deployment`, - ` + update: ` apiVersion: apps/v1 kind: StatefulSet`, - ` + local: ` apiVersion: apps/v1 list: # not present in sources - 2 - 3 - 4 `, - ` + expected: ` apiVersion: apps/v1 list: # not present in sources - 2 - 3 - 4 kind: StatefulSet -`, nil}, +`}, // // Test Case // // TODO(#36): consider making this an error - {`Change an updated field`, - ` + {description: `Change an updated field`, + origin: ` apiVersion: apps/v1 list: # old value - 1 - 2 - 3`, - ` + update: ` apiVersion: apps/v1 list: # new value - 2 - 3 - 4`, - ` + local: ` apiVersion: apps/v1 list: # conflicting value - a - b - c`, - ` + expected: ` apiVersion: apps/v1 list: # conflicting value - 2 - 3 - 4 -`, nil}, +`}, // // Test Case // - {`Ignore a field -- set`, - ` + {description: `Ignore a field -- set`, + origin: ` apiVersion: apps/v1 list: # ignore value - 1 - 2 - 3 `, - ` + update: ` apiVersion: apps/v1 list: # ignore value - 1 - 2 -- 3`, ` +- 3`, + local: ` apiVersion: apps/v1 list: - 2 - 3 - 4 -`, ` +`, + expected: ` apiVersion: apps/v1 list: - 2 - 3 - 4 -`, nil}, +`}, // // Test Case // - {`Ignore a field -- empty`, - ` + {description: `Ignore a field -- empty`, + origin: ` apiVersion: apps/v1 list: # ignore value - 1 - 2 - 3`, - ` + update: ` apiVersion: apps/v1 list: # ignore value - 1 - 2 - 3`, - ` + local: ` apiVersion: apps/v1 `, - ` + expected: ` apiVersion: apps/v1 -`, nil}, +`}, // // Test Case // - {`Explicitly clear a field`, - ` + {description: `Explicitly clear a field`, + origin: ` apiVersion: apps/v1`, - ` + update: ` apiVersion: apps/v1 list: null # clear`, - ` + local: ` apiVersion: apps/v1 list: # value to clear - 1 - 2 - 3`, - ` -apiVersion: apps/v1`, nil}, + expected: ` +apiVersion: apps/v1`}, // // Test Case // - {`Implicitly clear a field`, - ` + {description: `Implicitly clear a field`, + origin: ` apiVersion: apps/v1 list: # clear value - 1 - 2 - 3`, - ` + update: ` apiVersion: apps/v1`, - ` + local: ` apiVersion: apps/v1 list: # old value - 1 - 2 - 3`, - ` -apiVersion: apps/v1`, nil}, + expected: ` +apiVersion: apps/v1`}, // // Test Case // // TODO(#36): consider making this an error - {`Implicitly clear a changed field`, - ` + {description: `Implicitly clear a changed field`, + origin: ` apiVersion: apps/v1 list: # old value - 1 - 2 - 3`, - ` + update: ` apiVersion: apps/v1`, - ` + local: ` apiVersion: apps/v1 list: # old value - a - b - c`, - ` -apiVersion: apps/v1`, nil}, + expected: ` +apiVersion: apps/v1`}, } diff --git a/kyaml/yaml/merge3/map_test.go b/kyaml/yaml/merge3/map_test.go index e1ffd5413..be9827c0e 100644 --- a/kyaml/yaml/merge3/map_test.go +++ b/kyaml/yaml/merge3/map_test.go @@ -7,267 +7,267 @@ var mapTestCases = []testCase{ // // Test Case // - {`Add the annotations map field`, - ` + {description: `Add the annotations map field`, + origin: ` kind: Deployment`, - ` + update: ` kind: Deployment metadata: annotations: d: e # add these annotations `, - ` + local: ` kind: Deployment`, - ` + expected: ` kind: Deployment metadata: annotations: - d: e # add these annotations`, nil}, + d: e # add these annotations`}, // // Test Case // - {`Add an annotation to the field`, - ` + {description: `Add an annotation to the field`, + origin: ` kind: Deployment metadata: annotations: a: b`, - ` + update: ` kind: Deployment metadata: annotations: a: b d: e # add these annotations`, - ` + local: ` kind: Deployment metadata: annotations: g: h # keep these annotations`, - ` + expected: ` kind: Deployment metadata: annotations: g: h # keep these annotations - d: e # add these annotations`, nil}, + d: e # add these annotations`}, // // Test Case // - {`Add an annotation to the field, field missing from dest`, - ` + {description: `Add an annotation to the field, field missing from dest`, + origin: ` kind: Deployment metadata: annotations: a: b # ignored because unchanged`, - ` + update: ` kind: Deployment metadata: annotations: a: b # ignore because unchanged d: e`, - ` + local: ` kind: Deployment`, - ` + expected: ` kind: Deployment metadata: annotations: - d: e`, nil}, + d: e`}, // // Test Case // - {`Update an annotation on the field, field messing rom the dest`, - ` + {description: `Update an annotation on the field, field messing rom the dest`, + origin: ` kind: Deployment metadata: annotations: a: b d: c`, - ` + update: ` kind: Deployment metadata: annotations: a: b d: e # set these annotations`, - ` + local: ` kind: Deployment metadata: annotations: g: h # keep these annotations`, - ` + expected: ` kind: Deployment metadata: annotations: g: h # keep these annotations - d: e # set these annotations`, nil}, + d: e # set these annotations`}, // // Test Case // - {`Add an annotation to the field, field missing from dest`, - ` + {description: `Add an annotation to the field, field missing from dest`, + origin: ` kind: Deployment metadata: annotations: a: b # ignored because unchanged`, - ` + update: ` kind: Deployment metadata: annotations: a: b # ignore because unchanged d: e`, - ` + local: ` kind: Deployment`, - ` + expected: ` kind: Deployment metadata: annotations: - d: e`, nil}, + d: e`}, // // Test Case // - {`Remove an annotation`, - ` + {description: `Remove an annotation`, + origin: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: a: b`, - ` + update: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: {}`, - ` + local: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: c: d a: b`, - ` + expected: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: - c: d`, nil}, + c: d`}, // // Test Case // // TODO(#36) support ~annotations~: {} deletion - {`Specify a field as empty that isn't present in the source`, - ` + {description: `Specify a field as empty that isn't present in the source`, + origin: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo`, - ` + update: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo annotations: null`, - ` + local: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo annotations: a: b`, - ` + expected: ` apiVersion: apps/v1 kind: Deployment metadata: - name: foo`, nil}, + name: foo`}, // // Test Case // - {`Remove an annotation`, - ` + {description: `Remove an annotation`, + origin: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: a: b`, - ` + update: ` apiVersion: apps/v1 kind: Deployment`, - ` + local: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: c: d a: b`, - ` + expected: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: - c: d`, nil}, + c: d`}, // // Test Case // - {`Remove annotations field`, - ` + {description: `Remove annotations field`, + origin: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: a: b`, - ` + update: ` apiVersion: apps/v1 kind: Deployment`, - ` + local: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo`, - ` + expected: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo -`, nil}, +`}, // // Test Case // - {`Remove annotations field, but keep in dest`, - ` + {description: `Remove annotations field, but keep in dest`, + origin: ` apiVersion: apps/v1 kind: Deployment metadata: annotations: a: b`, - ` + update: ` apiVersion: apps/v1 kind: Deployment`, - ` + local: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo annotations: foo: bar # keep this annotation even though the parent field was removed`, - ` + expected: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo annotations: - foo: bar # keep this annotation even though the parent field was removed`, nil}, + foo: bar # keep this annotation even though the parent field was removed`}, // // Test Case // - {`Remove annotations, but they are already empty`, - ` + {description: `Remove annotations, but they are already empty`, + origin: ` apiVersion: apps/v1 kind: Deployment metadata: @@ -275,24 +275,24 @@ metadata: annotations: a: b `, - ` + update: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo `, - ` + local: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo annotations: {} `, - ` + expected: ` apiVersion: apps/v1 kind: Deployment metadata: name: foo annotations: {} -`, nil}, +`}, } diff --git a/kyaml/yaml/merge3/merge3.go b/kyaml/yaml/merge3/merge3.go index d35fee5ca..dc1900958 100644 --- a/kyaml/yaml/merge3/merge3.go +++ b/kyaml/yaml/merge3/merge3.go @@ -13,11 +13,12 @@ import ( func Merge(dest, original, update *yaml.RNode) (*yaml.RNode, error) { // if update == nil && original != nil => declarative deletion - return walk.Walker{Visitor: Visitor{}, + return walk.Walker{ + Visitor: Visitor{}, Sources: []*yaml.RNode{dest, original, update}}.Walk() } -func MergeStrings(dest, original, update string) (string, error) { +func MergeStrings(dest, original, update string, infer bool) (string, error) { srcOriginal, err := yaml.Parse(original) if err != nil { return "", err @@ -31,7 +32,10 @@ func MergeStrings(dest, original, update string) (string, error) { return "", err } - result, err := Merge(d, srcOriginal, srcUpdated) + result, err := walk.Walker{ + InferAssociativeLists: infer, + Visitor: Visitor{}, + Sources: []*yaml.RNode{d, srcOriginal, srcUpdated}}.Walk() if err != nil { return "", err } diff --git a/kyaml/yaml/merge3/merge3_test.go b/kyaml/yaml/merge3/merge3_test.go index 905e06cf2..ef882b48f 100644 --- a/kyaml/yaml/merge3/merge3_test.go +++ b/kyaml/yaml/merge3/merge3_test.go @@ -15,24 +15,27 @@ var testCases = [][]testCase{scalarTestCases, listTestCases, mapTestCases, eleme func TestMerge(t *testing.T) { for i := range testCases { - for _, tc := range testCases[i] { - actual, err := MergeStrings(tc.local, tc.origin, tc.update) - if tc.err == nil { - if !assert.NoError(t, err, tc.description) { - t.FailNow() + for j := range testCases[i] { + tc := testCases[i][j] + t.Run(tc.description, func(t *testing.T) { + actual, err := MergeStrings(tc.local, tc.origin, tc.update, !tc.noInfer) + if tc.err == nil { + if !assert.NoError(t, err, tc.description) { + t.FailNow() + } + if !assert.Equal(t, + strings.TrimSpace(tc.expected), strings.TrimSpace(actual), tc.description) { + t.FailNow() + } + } else { + if !assert.Errorf(t, err, tc.description) { + t.FailNow() + } + if !assert.Contains(t, tc.err.Error(), err.Error()) { + t.FailNow() + } } - if !assert.Equal(t, - strings.TrimSpace(tc.expected), strings.TrimSpace(actual), tc.description) { - t.FailNow() - } - } else { - if !assert.Errorf(t, err, tc.description) { - t.FailNow() - } - if !assert.Contains(t, tc.err.Error(), err.Error()) { - t.FailNow() - } - } + }) } } } @@ -44,4 +47,5 @@ type testCase struct { local string expected string err error + noInfer bool } diff --git a/kyaml/yaml/merge3/scalar_test.go b/kyaml/yaml/merge3/scalar_test.go index b74d4891d..c882df055 100644 --- a/kyaml/yaml/merge3/scalar_test.go +++ b/kyaml/yaml/merge3/scalar_test.go @@ -8,128 +8,128 @@ var scalarTestCases = []testCase{ // // Test Case // - {`Set and updated a field`, - `kind: Deployment`, - `kind: StatefulSet`, - `kind: Deployment`, - `kind: StatefulSet`, nil}, + {description: `Set and updated a field`, + origin: `kind: Deployment`, + update: `kind: StatefulSet`, + local: `kind: Deployment`, + expected: `kind: StatefulSet`}, - {`Add an updated field`, - ` + {description: `Add an updated field`, + origin: ` apiVersion: apps/v1 kind: Deployment # old value`, - ` + update: ` apiVersion: apps/v1 kind: StatefulSet # new value`, - ` + local: ` apiVersion: apps/v1`, - ` + expected: ` apiVersion: apps/v1 -kind: StatefulSet # new value`, nil}, +kind: StatefulSet # new value`}, - {`Add keep an omitted field`, - ` + {description: `Add keep an omitted field`, + origin: ` apiVersion: apps/v1 kind: Deployment`, - ` + update: ` apiVersion: apps/v1 kind: StatefulSet`, - ` + local: ` apiVersion: apps/v1 spec: foo # field not present in source `, - ` + expected: ` apiVersion: apps/v1 spec: foo # field not present in source kind: StatefulSet -`, nil}, +`}, // // Test Case // // TODO(#36): consider making this an error - {`Change an updated field`, - ` + {description: `Change an updated field`, + origin: ` apiVersion: apps/v1 kind: Deployment # old value`, - ` + update: ` apiVersion: apps/v1 kind: StatefulSet # new value`, - ` + local: ` apiVersion: apps/v1 kind: Service # conflicting value`, - ` + expected: ` apiVersion: apps/v1 -kind: StatefulSet # new value`, nil}, +kind: StatefulSet # new value`}, - {`Ignore a field`, - ` + {description: `Ignore a field`, + origin: ` apiVersion: apps/v1 kind: Deployment # ignore this field`, - ` + update: ` apiVersion: apps/v1 kind: Deployment # ignore this field`, - ` + local: ` apiVersion: apps/v1`, - ` -apiVersion: apps/v1`, nil}, + expected: ` +apiVersion: apps/v1`}, - {`Explicitly clear a field`, - ` + {description: `Explicitly clear a field`, + origin: ` apiVersion: apps/v1`, - ` + update: ` apiVersion: apps/v1 kind: null # clear this value`, - ` + local: ` apiVersion: apps/v1 kind: Deployment # value to be cleared`, - ` -apiVersion: apps/v1`, nil}, + expected: ` +apiVersion: apps/v1`}, - {`Implicitly clear a field`, - ` + {description: `Implicitly clear a field`, + origin: ` apiVersion: apps/v1 kind: Deployment # clear this field`, - ` + update: ` apiVersion: apps/v1`, - ` + local: ` apiVersion: apps/v1 kind: Deployment # clear this field`, - ` -apiVersion: apps/v1`, nil}, + expected: ` +apiVersion: apps/v1`}, // // Test Case // // TODO(#36): consider making this an error - {`Implicitly clear a changed field`, - ` + {description: `Implicitly clear a changed field`, + origin: ` apiVersion: apps/v1 kind: Deployment`, - ` + update: ` apiVersion: apps/v1`, - ` + local: ` apiVersion: apps/v1 kind: StatefulSet`, - ` -apiVersion: apps/v1`, nil}, + expected: ` +apiVersion: apps/v1`}, // // Test Case // - {`Merge an empty scalar value`, - ` + {description: `Merge an empty scalar value`, + origin: ` apiVersion: apps/v1 `, - ` + update: ` apiVersion: apps/v1 kind: {} `, - ` + local: ` apiVersion: apps/v1 `, - ` + expected: ` apiVersion: apps/v1 kind: {} -`, nil}, +`}, } diff --git a/kyaml/yaml/merge3/visitor.go b/kyaml/yaml/merge3/visitor.go index 7e2f12cc8..f2cb2f79d 100644 --- a/kyaml/yaml/merge3/visitor.go +++ b/kyaml/yaml/merge3/visitor.go @@ -4,6 +4,7 @@ package merge3 import ( + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" "sigs.k8s.io/kustomize/kyaml/yaml/walk" ) @@ -17,7 +18,7 @@ const ( type Visitor struct{} -func (m Visitor) VisitMap(nodes walk.Sources) (*yaml.RNode, error) { +func (m Visitor) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { if yaml.IsNull(nodes.Updated()) || yaml.IsNull(nodes.Dest()) { // explicitly cleared from either dest or update return walk.ClearNode, nil @@ -36,7 +37,7 @@ func (m Visitor) VisitMap(nodes walk.Sources) (*yaml.RNode, error) { return nodes.Dest(), nil } -func (m Visitor) visitAList(nodes walk.Sources) (*yaml.RNode, error) { +func (m Visitor) visitAList(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { if yaml.IsEmpty(nodes.Updated()) && !yaml.IsEmpty(nodes.Origin()) { // implicitly cleared from update -- element was deleted return walk.ClearNode, nil @@ -51,7 +52,7 @@ func (m Visitor) visitAList(nodes walk.Sources) (*yaml.RNode, error) { return nodes.Dest(), nil } -func (m Visitor) VisitScalar(nodes walk.Sources) (*yaml.RNode, error) { +func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { if yaml.IsNull(nodes.Updated()) || yaml.IsNull(nodes.Dest()) { // explicitly cleared from either dest or update return nil, nil @@ -103,9 +104,9 @@ func (m Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { return nodes.Dest(), nil } -func (m Visitor) VisitList(nodes walk.Sources, kind walk.ListKind) (*yaml.RNode, error) { +func (m Visitor) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, kind walk.ListKind) (*yaml.RNode, error) { if kind == walk.AssociativeList { - return m.visitAList(nodes) + return m.visitAList(nodes, s) } // non-associative list return m.visitNAList(nodes) diff --git a/kyaml/yaml/schema/schema.go b/kyaml/yaml/schema/schema.go new file mode 100644 index 000000000..908d050a3 --- /dev/null +++ b/kyaml/yaml/schema/schema.go @@ -0,0 +1,34 @@ +// Copyright 2019 The Kubernetes Authors. +// SPDX-License-Identifier: Apache-2.0 + +// Package schema contains libraries for working with the yaml and openapi packages. +package schema + +import ( + "sigs.k8s.io/kustomize/kyaml/openapi" + "sigs.k8s.io/kustomize/kyaml/yaml" +) + +// IsAssociative returns true if all elements in the list contain an AssociativeSequenceKey +// as a field. +func IsAssociative(schema *openapi.ResourceSchema, nodes []*yaml.RNode, infer bool) bool { + if schema != nil { + // use the schema to identify if the list is associative + s, _ := schema.PatchStrategyAndKey() + return s == "merge" + } + if !infer { + return false + } + + for i := range nodes { + node := nodes[i] + if yaml.IsEmpty(node) { + continue + } + if node.IsAssociative() { + return true + } + } + return false +} diff --git a/kyaml/yaml/types.go b/kyaml/yaml/types.go index 9f740ef40..cedf1f878 100644 --- a/kyaml/yaml/types.go +++ b/kyaml/yaml/types.go @@ -651,24 +651,8 @@ func (rn *RNode) VisitElements(fn func(node *RNode) error) error { // The order sets the precedence of the merge keys -- if multiple keys are present // in Resources in a list, then the FIRST key which ALL elements in the list have is used as the // associative key for merging that list. -var AssociativeSequenceKeys = []string{ - "mountPath", "devicePath", "ip", "type", "topologyKey", "name", "containerPort", -} - -// IsAssociative returns true if all elements in the list contain an AssociativeSequenceKey -// as a field. -func IsAssociative(nodes []*RNode) bool { - for i := range nodes { - node := nodes[i] - if IsEmpty(node) { - continue - } - if node.IsAssociative() { - return true - } - } - return false -} +// Only infer name as a merge key. +var AssociativeSequenceKeys = []string{"name"} // IsAssociative returns true if the RNode contains an AssociativeSequenceKey as a field. func (rn *RNode) IsAssociative() bool { diff --git a/kyaml/yaml/walk/associative_sequence.go b/kyaml/yaml/walk/associative_sequence.go index 1ef341607..7d4a219f1 100644 --- a/kyaml/yaml/walk/associative_sequence.go +++ b/kyaml/yaml/walk/associative_sequence.go @@ -7,28 +7,44 @@ import ( "strings" "github.com/go-errors/errors" + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/sets" "sigs.k8s.io/kustomize/kyaml/yaml" ) func (l *Walker) walkAssociativeSequence() (*yaml.RNode, error) { // may require initializing the dest node - dest, err := l.Sources.setDestNode(l.VisitList(l.Sources, AssociativeList)) + dest, err := l.Sources.setDestNode(l.VisitList(l.Sources, l.Schema, AssociativeList)) if dest == nil || err != nil { return nil, err } - // find the list of elements we need to recursively walk - key, err := l.elementKey() - if err != nil { - return nil, err + var key string + if l.Schema != nil { + _, key = l.Schema.PatchStrategyAndKey() } + if key == "" { // no key from the schema, try to infer one + // find the list of elements we need to recursively walk + key, err = l.elementKey() + if err != nil { + return nil, err + } + } + values := l.elementValues(key) // recursively set the elements in the list + var s *openapi.ResourceSchema + if l.Schema != nil { + s = l.Schema.Elements() + } for _, value := range values { - val, err := Walker{Visitor: l, - Sources: l.elementValue(key, value)}.Walk() + val, err := Walker{ + InferAssociativeLists: l.InferAssociativeLists, + Visitor: l, + Schema: s, + Sources: l.elementValue(key, value), + }.Walk() if err != nil { return nil, err } diff --git a/kyaml/yaml/walk/map.go b/kyaml/yaml/walk/map.go index 375c807c2..188aed9fa 100644 --- a/kyaml/yaml/walk/map.go +++ b/kyaml/yaml/walk/map.go @@ -6,6 +6,8 @@ package walk import ( "sort" + "sigs.k8s.io/kustomize/kyaml/fieldmeta" + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/sets" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -18,15 +20,27 @@ import ( // - set each source field value on l.Dest func (l Walker) walkMap() (*yaml.RNode, error) { // get the new map value - dest, err := l.Sources.setDestNode(l.VisitMap(l.Sources)) + dest, err := l.Sources.setDestNode(l.VisitMap(l.Sources, l.Schema)) if dest == nil || err != nil { return nil, err } // recursively set the field values on the map for _, key := range l.fieldNames() { - val, err := Walker{Visitor: l, - Sources: l.fieldValue(key), Path: append(l.Path, key)}.Walk() + var s *openapi.ResourceSchema + if l.Schema != nil { + s = l.Schema.Field(key) + } + fv, commentSch := l.fieldValue(key) + if commentSch != nil { + s = commentSch + } + val, err := Walker{ + InferAssociativeLists: l.InferAssociativeLists, + Visitor: l, + Schema: s, + Sources: fv, + Path: append(l.Path, key)}.Walk() if err != nil { return nil, err } @@ -42,11 +56,40 @@ func (l Walker) walkMap() (*yaml.RNode, error) { } // valueIfPresent returns node.Value if node is non-nil, otherwise returns nil -func (l Walker) valueIfPresent(node *yaml.MapNode) *yaml.RNode { +func (l Walker) valueIfPresent(node *yaml.MapNode) (*yaml.RNode, *openapi.ResourceSchema) { if node == nil { - return nil + return nil, nil } - return node.Value + + // parse the schema for the field if present + var s *openapi.ResourceSchema + fm := fieldmeta.FieldMeta{} + var err error + // check the value for a schema + if err = fm.Read(node.Value); err == nil { + s = &openapi.ResourceSchema{Schema: &fm.Schema} + if fm.Schema.Ref.String() != "" { + r, err := openapi.Resolve(&fm.Schema.Ref) + if err == nil && r != nil { + s.Schema = r + } + } + } + // check the key for a schema -- this will be used + // when the value is a Sequence (comments are attached) + // to the key + if fm.IsEmpty() { + if err = fm.Read(node.Key); err == nil { + s = &openapi.ResourceSchema{Schema: &fm.Schema} + } + if fm.Schema.Ref.String() != "" { + r, err := openapi.Resolve(&fm.Schema.Ref) + if err == nil && r != nil { + s.Schema = r + } + } + } + return node.Value, s } // fieldNames returns a sorted slice containing the names of all fields that appear in any of @@ -67,15 +110,20 @@ func (l Walker) fieldNames() []string { } // fieldValue returns a slice containing each source's value for fieldName -func (l Walker) fieldValue(fieldName string) []*yaml.RNode { +func (l Walker) fieldValue(fieldName string) ([]*yaml.RNode, *openapi.ResourceSchema) { var fields []*yaml.RNode + var sch *openapi.ResourceSchema for i := range l.Sources { if l.Sources[i] == nil { fields = append(fields, nil) continue } field := l.Sources[i].Field(fieldName) - fields = append(fields, l.valueIfPresent(field)) + f, s := l.valueIfPresent(field) + fields = append(fields, f) + if sch == nil && !s.IsEmpty() { + sch = s + } } - return fields + return fields, sch } diff --git a/kyaml/yaml/walk/nonassociative_sequence.go b/kyaml/yaml/walk/nonassociative_sequence.go index 7a34da916..91b187e5b 100644 --- a/kyaml/yaml/walk/nonassociative_sequence.go +++ b/kyaml/yaml/walk/nonassociative_sequence.go @@ -9,5 +9,5 @@ import ( // walkNonAssociativeSequence returns the value of VisitList func (l Walker) walkNonAssociativeSequence() (*yaml.RNode, error) { - return l.VisitList(l.Sources, NonAssociateList) + return l.VisitList(l.Sources, l.Schema, NonAssociateList) } diff --git a/kyaml/yaml/walk/scalar.go b/kyaml/yaml/walk/scalar.go index 18d63be46..1a26f6dff 100644 --- a/kyaml/yaml/walk/scalar.go +++ b/kyaml/yaml/walk/scalar.go @@ -7,5 +7,5 @@ import "sigs.k8s.io/kustomize/kyaml/yaml" // walkScalar returns the value of VisitScalar func (l Walker) walkScalar() (*yaml.RNode, error) { - return l.VisitScalar(l.Sources) + return l.VisitScalar(l.Sources, l.Schema) } diff --git a/kyaml/yaml/walk/visitor.go b/kyaml/yaml/walk/visitor.go index eb1451bbf..153ac2945 100644 --- a/kyaml/yaml/walk/visitor.go +++ b/kyaml/yaml/walk/visitor.go @@ -4,6 +4,7 @@ package walk import ( + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" ) @@ -16,11 +17,11 @@ const ( // Visitor is invoked by walk with source and destination node pairs type Visitor interface { - VisitMap(nodes Sources) (*yaml.RNode, error) + VisitMap(Sources, *openapi.ResourceSchema) (*yaml.RNode, error) - VisitScalar(nodes Sources) (*yaml.RNode, error) + VisitScalar(Sources, *openapi.ResourceSchema) (*yaml.RNode, error) - VisitList(nodes Sources, kind ListKind) (*yaml.RNode, error) + VisitList(Sources, *openapi.ResourceSchema, ListKind) (*yaml.RNode, error) } // ClearNode is returned if GrepFilter should do nothing after calling Set diff --git a/kyaml/yaml/walk/walk.go b/kyaml/yaml/walk/walk.go index 20e4979b8..2040aabd3 100644 --- a/kyaml/yaml/walk/walk.go +++ b/kyaml/yaml/walk/walk.go @@ -8,20 +8,30 @@ import ( "os" "strings" + "sigs.k8s.io/kustomize/kyaml/fieldmeta" + "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" + "sigs.k8s.io/kustomize/kyaml/yaml/schema" ) -// Filter walks the Source RNode and modifies the RNode provided to GrepFilter. +// Walker walks the Source RNode and modifies the RNode provided to GrepFilter. type Walker struct { // Visitor is invoked by GrepFilter Visitor + Schema *openapi.ResourceSchema + // Source is the RNode to walk. All Source fields and associative list elements // will be visited. Sources Sources // Path is the field path to the current Source Node. Path []string + + // InferAssociativeLists if set to true will infer merge strategies for + // fields which it doesn't have the schema based on the fields in the + // list elements. + InferAssociativeLists bool } func (l Walker) Kind() yaml.Kind { @@ -35,6 +45,8 @@ func (l Walker) Kind() yaml.Kind { // GrepFilter implements yaml.GrepFilter func (l Walker) Walk() (*yaml.RNode, error) { + l.Schema = l.GetSchema() + // invoke the handler for the corresponding node type switch l.Kind() { case yaml.MappingNode: @@ -46,7 +58,7 @@ func (l Walker) Walk() (*yaml.RNode, error) { if err := yaml.ErrorIfAnyInvalidAndNonNull(yaml.SequenceNode, l.Sources...); err != nil { return nil, err } - if yaml.IsAssociative(l.Sources) { + if schema.IsAssociative(l.Schema, l.Sources, l.InferAssociativeLists) { return l.walkAssociativeSequence() } return l.walkNonAssociativeSequence() @@ -64,6 +76,49 @@ func (l Walker) Walk() (*yaml.RNode, error) { } } +func (l Walker) GetSchema() *openapi.ResourceSchema { + for i := range l.Sources { + r := l.Sources[i] + if yaml.IsEmpty(r) { + continue + } + + fm := fieldmeta.FieldMeta{} + if err := fm.Read(r); err == nil && !fm.IsEmpty() { + // per-field schema, this is fine + if fm.Schema.Ref.String() != "" { + // resolve the reference + s, err := openapi.Resolve(&fm.Schema.Ref) + if err == nil && s != nil { + fm.Schema = *s + } + } + return &openapi.ResourceSchema{Schema: &fm.Schema} + } + } + + if l.Schema != nil { + return l.Schema + } + for i := range l.Sources { + r := l.Sources[i] + if yaml.IsEmpty(r) { + continue + } + + m, _ := r.GetMeta() + if m.Kind == "" || m.APIVersion == "" { + continue + } + + s := openapi.SchemaForResourceType(yaml.TypeMeta{Kind: m.Kind, APIVersion: m.APIVersion}) + if s != nil { + return s + } + } + return nil +} + const ( DestIndex = iota OriginIndex