From 4e74947731f0a55a7116a1f0eb405c911362885f Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Sun, 2 Aug 2020 18:24:56 -0700 Subject: [PATCH] Update setter comments during 3-way merge --- kyaml/yaml/merge3/element_test.go | 60 +++++++---- kyaml/yaml/merge3/kustomization_test.go | 6 +- kyaml/yaml/merge3/list_test.go | 27 +++-- kyaml/yaml/merge3/map_test.go | 37 ++++--- kyaml/yaml/merge3/scalar_test.go | 133 +++++++++++++++++++++--- kyaml/yaml/merge3/visitor.go | 15 +++ 6 files changed, 222 insertions(+), 56 deletions(-) diff --git a/kyaml/yaml/merge3/element_test.go b/kyaml/yaml/merge3/element_test.go index 284c580a2..84dc84154 100644 --- a/kyaml/yaml/merge3/element_test.go +++ b/kyaml/yaml/merge3/element_test.go @@ -8,7 +8,8 @@ var elementTestCases = []testCase{ // // Test Case // - {description: `Add an element to an existing list`, + { + description: `Add an element to an existing list`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -57,7 +58,8 @@ spec: // // Test Case // - {description: `Add an element to a non-existing list`, + { + description: `Add an element to a non-existing list`, origin: ` apiVersion: apps/v1 kind: Deployment`, @@ -86,7 +88,8 @@ spec: name: foo `}, - {description: `Add an element to a non-existing list, existing in dest`, + { + description: `Add an element to a non-existing list, existing in dest`, origin: ` apiVersion: apps/v1 kind: Deployment`, @@ -127,7 +130,8 @@ spec: // 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 - {description: `Add a field to the element, element missing from dest`, + { + description: `Add a field to the element, element missing from dest`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -168,7 +172,8 @@ spec: // // Test Case // - {description: `Update a field on the elem, element missing from the dest`, + { + description: `Update a field on the elem, element missing from the dest`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -212,7 +217,8 @@ spec: // // Test Case // - {description: `Update a field on the elem, element present in the dest`, + { + description: `Update a field on the elem, element present in the dest`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -308,7 +314,8 @@ spec: // // Test Case // - {description: `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: ` apiVersion: apps/v1 kind: Deployment @@ -356,7 +363,8 @@ spec: // // Test Case // - {description: `Ignore an element`, + { + description: `Ignore an element`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -397,7 +405,8 @@ spec: // // Test Case // - {description: `Leave deleted`, + { + description: `Leave deleted`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -424,7 +433,8 @@ kind: Deployment // // Test Case // - {description: `Remove an element -- matching`, + { + description: `Remove an element -- matching`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -463,7 +473,8 @@ spec: // // Test Case // - {description: `Remove an element -- field missing from update`, + { + description: `Remove an element -- field missing from update`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -503,7 +514,8 @@ spec: // // Test Case // - {description: `Remove an element -- element missing`, + { + description: `Remove an element -- element missing`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -554,7 +566,8 @@ spec: // // Test Case // - {description: `Remove an element -- empty containers`, + { + description: `Remove an element -- empty containers`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -595,7 +608,8 @@ spec: // // Test Case // - {description: `Remove an element -- missing list field`, + { + description: `Remove an element -- missing list field`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -635,7 +649,8 @@ spec: // // Test Case // - {description: `infer merge keys merge'`, + { + description: `infer merge keys merge'`, origin: ` kind: Deployment spec: @@ -678,7 +693,8 @@ spec: // // Test Case // - {description: `no infer merge keys merge using schema`, + { + description: `no infer merge keys merge using schema`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -725,7 +741,8 @@ spec: // // Test Case // - {description: `no infer merge keys merge using explicit schema as line comment'`, + { + description: `no infer merge keys merge using explicit schema as line comment'`, origin: ` apiVersion: custom kind: Deployment @@ -772,7 +789,8 @@ spec: // // Test Case // - {description: `no infer merge keys merge using explicit schema as head comment'`, + { + description: `no infer merge keys merge using explicit schema as head comment'`, origin: ` apiVersion: custom kind: Deployment @@ -821,7 +839,8 @@ spec: // // Test Case // - {description: `no infer merge keys merge using explicit schema to parent field'`, + { + description: `no infer merge keys merge using explicit schema to parent field'`, origin: ` apiVersion: custom kind: Deployment @@ -860,7 +879,8 @@ spec: # {"$ref":"#/definitions/io.k8s.api.core.v1.PodSpec"} // // Test Case // - {description: `no infer merge keys merge using explicit schema to parent field header'`, + { + description: `no infer merge keys merge using explicit schema to parent field header'`, origin: ` apiVersion: custom kind: Deployment diff --git a/kyaml/yaml/merge3/kustomization_test.go b/kyaml/yaml/merge3/kustomization_test.go index 28cbc751c..4eeac4f97 100644 --- a/kyaml/yaml/merge3/kustomization_test.go +++ b/kyaml/yaml/merge3/kustomization_test.go @@ -6,7 +6,8 @@ package merge3_test var kustomizationTestCases = []testCase{ // Kustomization Test Cases - {description: `ConfigMapGenerator merge`, + { + description: `ConfigMapGenerator merge`, origin: ` apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization @@ -48,7 +49,8 @@ configMapGenerator: - configkey=configs/another_configfile2 name: a-configmap2`}, - {description: `SecretGenerator merge`, + { + description: `SecretGenerator merge`, origin: ` apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization diff --git a/kyaml/yaml/merge3/list_test.go b/kyaml/yaml/merge3/list_test.go index 467c9e5b9..17ab00f08 100644 --- a/kyaml/yaml/merge3/list_test.go +++ b/kyaml/yaml/merge3/list_test.go @@ -9,7 +9,8 @@ var listTestCases = []testCase{ // // Test Case // - {description: `Replace list`, + { + description: `Replace list`, origin: ` list: - 1 @@ -34,7 +35,8 @@ list: // // Test Case // - {description: `Add an updated list`, + { + description: `Add an updated list`, origin: ` apiVersion: apps/v1 list: # old value @@ -62,7 +64,8 @@ list: // // Test Case // - {description: `Add keep an omitted field`, + { + description: `Add keep an omitted field`, origin: ` apiVersion: apps/v1 kind: Deployment`, @@ -89,7 +92,8 @@ kind: StatefulSet // Test Case // // TODO(#36): consider making this an error - {description: `Change an updated field`, + { + description: `Change an updated field`, origin: ` apiVersion: apps/v1 list: # old value @@ -119,7 +123,8 @@ list: # conflicting value // // Test Case // - {description: `Ignore a field -- set`, + { + description: `Ignore a field -- set`, origin: ` apiVersion: apps/v1 list: # ignore value @@ -151,7 +156,8 @@ list: // // Test Case // - {description: `Ignore a field -- empty`, + { + description: `Ignore a field -- empty`, origin: ` apiVersion: apps/v1 list: # ignore value @@ -174,7 +180,8 @@ apiVersion: apps/v1 // // Test Case // - {description: `Explicitly clear a field`, + { + description: `Explicitly clear a field`, origin: ` apiVersion: apps/v1`, update: ` @@ -192,7 +199,8 @@ apiVersion: apps/v1`}, // // Test Case // - {description: `Implicitly clear a field`, + { + description: `Implicitly clear a field`, origin: ` apiVersion: apps/v1 list: # clear value @@ -214,7 +222,8 @@ apiVersion: apps/v1`}, // Test Case // // TODO(#36): consider making this an error - {description: `Implicitly clear a changed field`, + { + description: `Implicitly clear a changed field`, origin: ` apiVersion: apps/v1 list: # old value diff --git a/kyaml/yaml/merge3/map_test.go b/kyaml/yaml/merge3/map_test.go index be9827c0e..4875ae76c 100644 --- a/kyaml/yaml/merge3/map_test.go +++ b/kyaml/yaml/merge3/map_test.go @@ -7,7 +7,8 @@ var mapTestCases = []testCase{ // // Test Case // - {description: `Add the annotations map field`, + { + description: `Add the annotations map field`, origin: ` kind: Deployment`, update: ` @@ -27,7 +28,8 @@ metadata: // // Test Case // - {description: `Add an annotation to the field`, + { + description: `Add an annotation to the field`, origin: ` kind: Deployment metadata: @@ -54,7 +56,8 @@ metadata: // // Test Case // - {description: `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: @@ -64,7 +67,7 @@ metadata: kind: Deployment metadata: annotations: - a: b # ignore because unchanged + a: b # ignored because unchanged d: e`, local: ` kind: Deployment`, @@ -77,7 +80,8 @@ metadata: // // Test Case // - {description: `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: @@ -105,7 +109,8 @@ metadata: // // Test Case // - {description: `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: @@ -115,7 +120,7 @@ metadata: kind: Deployment metadata: annotations: - a: b # ignore because unchanged + a: b # ignored because unchanged d: e`, local: ` kind: Deployment`, @@ -128,7 +133,8 @@ metadata: // // Test Case // - {description: `Remove an annotation`, + { + description: `Remove an annotation`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -158,7 +164,8 @@ metadata: // Test Case // // TODO(#36) support ~annotations~: {} deletion - {description: `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 @@ -186,7 +193,8 @@ metadata: // // Test Case // - {description: `Remove an annotation`, + { + description: `Remove an annotation`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -213,7 +221,8 @@ metadata: // // Test Case // - {description: `Remove annotations field`, + { + description: `Remove annotations field`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -238,7 +247,8 @@ metadata: // // Test Case // - {description: `Remove annotations field, but keep in dest`, + { + description: `Remove annotations field, but keep in dest`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -266,7 +276,8 @@ metadata: // // Test Case // - {description: `Remove annotations, but they are already empty`, + { + description: `Remove annotations, but they are already empty`, origin: ` apiVersion: apps/v1 kind: Deployment diff --git a/kyaml/yaml/merge3/scalar_test.go b/kyaml/yaml/merge3/scalar_test.go index c882df055..f488a8802 100644 --- a/kyaml/yaml/merge3/scalar_test.go +++ b/kyaml/yaml/merge3/scalar_test.go @@ -8,13 +8,15 @@ var scalarTestCases = []testCase{ // // Test Case // - {description: `Set and updated a field`, - origin: `kind: Deployment`, - update: `kind: StatefulSet`, - local: `kind: Deployment`, - expected: `kind: StatefulSet`}, + { + description: `Set and updated a field`, + origin: `kind: Deployment`, + update: `kind: StatefulSet`, + local: `kind: Deployment`, + expected: `kind: StatefulSet`}, - {description: `Add an updated field`, + { + description: `Add an updated field`, origin: ` apiVersion: apps/v1 kind: Deployment # old value`, @@ -27,7 +29,109 @@ apiVersion: apps/v1`, apiVersion: apps/v1 kind: StatefulSet # new value`}, - {description: `Add keep an omitted field`, + // + // Test Case + // + { + description: `Ensure comments are updated`, + origin: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'dest' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 3`, + update: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'updated' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 3 # {"$openapi":"replicas"}`, + local: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'original' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 3 +`, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'updated' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 3 # {"$openapi":"replicas"} +`}, + + { + description: `Ensure deleted comments are not updated`, + origin: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'dest' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 3 # {"$openapi":"replicas"}`, + update: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'updated' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 3 # {"$openapi":"replicas"}`, + local: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'original' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 4 +`, + expected: ` +apiVersion: apps/v1 +kind: Deployment +metadata: + name: nginx-deployment + annotations: + config.kubernetes.io/index: '0' + config.kubernetes.io/merge-source: 'updated' + config.kubernetes.io/path: 'temp.yaml' +spec: + replicas: 4 +`}, + + { + description: `Add keep an omitted field`, origin: ` apiVersion: apps/v1 kind: Deployment`, @@ -48,7 +152,8 @@ kind: StatefulSet // Test Case // // TODO(#36): consider making this an error - {description: `Change an updated field`, + { + description: `Change an updated field`, origin: ` apiVersion: apps/v1 kind: Deployment # old value`, @@ -62,7 +167,8 @@ kind: Service # conflicting value`, apiVersion: apps/v1 kind: StatefulSet # new value`}, - {description: `Ignore a field`, + { + description: `Ignore a field`, origin: ` apiVersion: apps/v1 kind: Deployment # ignore this field`, @@ -74,7 +180,8 @@ apiVersion: apps/v1`, expected: ` apiVersion: apps/v1`}, - {description: `Explicitly clear a field`, + { + description: `Explicitly clear a field`, origin: ` apiVersion: apps/v1`, update: ` @@ -102,7 +209,8 @@ apiVersion: apps/v1`}, // Test Case // // TODO(#36): consider making this an error - {description: `Implicitly clear a changed field`, + { + description: `Implicitly clear a changed field`, origin: ` apiVersion: apps/v1 kind: Deployment`, @@ -117,7 +225,8 @@ apiVersion: apps/v1`}, // // Test Case // - {description: `Merge an empty scalar value`, + { + description: `Merge an empty scalar value`, origin: ` apiVersion: apps/v1 `, diff --git a/kyaml/yaml/merge3/visitor.go b/kyaml/yaml/merge3/visitor.go index f2cb2f79d..3ae382b8b 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/errors" "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" "sigs.k8s.io/kustomize/kyaml/yaml/walk" @@ -66,6 +67,20 @@ func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*ya return nodes.Dest(), nil } + updatedStr, err := nodes.Updated().String() + if err != nil { + return nil, errors.Wrap(err) + } + originStr, err := nodes.Origin().String() + if err != nil { + return nil, errors.Wrap(err) + } + + if updatedStr != originStr { + // change in update node + return nodes.Updated(), nil + } + if nodes.Updated().YNode().Value != nodes.Origin().YNode().Value { // value changed in update return nodes.Updated(), nil