From 25e30de2d64427b591055e1fb10f3ed70e43b207 Mon Sep 17 00:00:00 2001 From: Phani Teja Marupaka Date: Sat, 15 Aug 2020 02:00:03 -0700 Subject: [PATCH] Update setter comments correctly on updates --- kyaml/yaml/fns.go | 6 +- kyaml/yaml/merge3/list_test.go | 102 ++++++++++++++++++++++++++++++- kyaml/yaml/merge3/merge3.go | 6 +- kyaml/yaml/merge3/scalar_test.go | 54 +++++++++++++++- kyaml/yaml/merge3/visitor.go | 14 ++--- kyaml/yaml/types.go | 8 ++- kyaml/yaml/walk/map.go | 22 ++++++- 7 files changed, 192 insertions(+), 20 deletions(-) diff --git a/kyaml/yaml/fns.go b/kyaml/yaml/fns.go index 6b18cf727..dbb122e46 100644 --- a/kyaml/yaml/fns.go +++ b/kyaml/yaml/fns.go @@ -447,6 +447,9 @@ type FieldSetter struct { // value on the ScalarNode. Name string `yaml:"name,omitempty"` + // Comments for the field + Comments Comments `yaml:"comments,omitempty"` + // Value is the value to set. // Optional if Kind is set. Value *RNode `yaml:"value,omitempty"` @@ -504,7 +507,8 @@ func (s FieldSetter) Filter(rn *RNode) (*RNode, error) { // create the field rn.YNode().Content = append(rn.YNode().Content, - &yaml.Node{Kind: yaml.ScalarNode, Value: s.Name}, + &yaml.Node{Kind: yaml.ScalarNode, HeadComment: s.Comments.HeadComment, + LineComment: s.Comments.LineComment, FootComment: s.Comments.FootComment, Value: s.Name}, s.Value.YNode()) return s.Value, nil } diff --git a/kyaml/yaml/merge3/list_test.go b/kyaml/yaml/merge3/list_test.go index 17ab00f08..72edfbd66 100644 --- a/kyaml/yaml/merge3/list_test.go +++ b/kyaml/yaml/merge3/list_test.go @@ -55,12 +55,108 @@ list: # new value apiVersion: apps/v1`, expected: ` apiVersion: apps/v1 -list: +list: # new value - 2 - 3 - 4 `}, + // + // Test Case + // + { + description: `Update comment`, + origin: ` +list: # comment +- 1 +- 2 +- 3`, + update: ` +list: # updated comment +- 2 +- 3 +- 4`, + local: ` +list: # comment +- 1 +- 2 +- 3`, + expected: ` +list: # updated comment +- 2 +- 3 +- 4`}, + + // + // Test Case + // + { + description: `Don't update local modified comment`, + origin: ` +list: # origin comment +- 1 +- 2 +- 3`, + update: ` +list: # updated comment +- 2 +- 3 +- 4`, + local: ` +list: # local comment +- 1 +- 2 +- 3`, + expected: ` +list: # local comment +- 2 +- 3 +- 4`}, + + // + // Test Case + // + { + description: `Don't add local deleted comment`, + origin: ` +list: # origin comment +- 1 +- 2 +- 3`, + update: ` +list: # updated comment +- 2 +- 3 +- 4`, + local: ` +list: +- 1 +- 2 +- 3`, + expected: ` +list: +- 2 +- 3 +- 4`}, + + { + description: `Add update with comment`, + origin: ` +apiVersion: apps/v1 +`, + update: ` +list: # updated comment +- 2 +- 3 +- 4`, + local: ` +apiVersion: apps/v1`, + expected: ` +list: # updated comment +- 2 +- 3 +- 4`}, + // // Test Case // @@ -140,14 +236,14 @@ list: # ignore value - 3`, local: ` apiVersion: apps/v1 -list: +list: # local comment - 2 - 3 - 4 `, expected: ` apiVersion: apps/v1 -list: +list: # local comment - 2 - 3 - 4 diff --git a/kyaml/yaml/merge3/merge3.go b/kyaml/yaml/merge3/merge3.go index dc1900958..664270e5b 100644 --- a/kyaml/yaml/merge3/merge3.go +++ b/kyaml/yaml/merge3/merge3.go @@ -14,8 +14,9 @@ func Merge(dest, original, update *yaml.RNode) (*yaml.RNode, error) { // if update == nil && original != nil => declarative deletion return walk.Walker{ - Visitor: Visitor{}, - Sources: []*yaml.RNode{dest, original, update}}.Walk() + Visitor: Visitor{}, + VisitKeysAsScalars: true, + Sources: []*yaml.RNode{dest, original, update}}.Walk() } func MergeStrings(dest, original, update string, infer bool) (string, error) { @@ -35,6 +36,7 @@ func MergeStrings(dest, original, update string, infer bool) (string, error) { result, err := walk.Walker{ InferAssociativeLists: infer, Visitor: Visitor{}, + VisitKeysAsScalars: true, Sources: []*yaml.RNode{d, srcOriginal, srcUpdated}}.Walk() if err != nil { return "", err diff --git a/kyaml/yaml/merge3/scalar_test.go b/kyaml/yaml/merge3/scalar_test.go index f488a8802..a3aa85153 100644 --- a/kyaml/yaml/merge3/scalar_test.go +++ b/kyaml/yaml/merge3/scalar_test.go @@ -33,7 +33,7 @@ kind: StatefulSet # new value`}, // Test Case // { - description: `Ensure comments are updated`, + description: `Ensure comments are added`, origin: ` apiVersion: apps/v1 kind: Deployment @@ -81,6 +81,58 @@ spec: replicas: 3 # {"$openapi":"replicas"} `}, + // + // 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 # {"$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_new"}`, + 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 # {"$openapi":"replicas"} +`, + 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_new"} +`}, + { description: `Ensure deleted comments are not updated`, origin: ` diff --git a/kyaml/yaml/merge3/visitor.go b/kyaml/yaml/merge3/visitor.go index 0d68907b9..978deff0c 100644 --- a/kyaml/yaml/merge3/visitor.go +++ b/kyaml/yaml/merge3/visitor.go @@ -4,7 +4,6 @@ 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" @@ -34,6 +33,7 @@ func (m Visitor) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml. // initialize a new value that can be recursively merged return yaml.NewRNode(&yaml.Node{Kind: yaml.MappingNode}), nil } + // recursively merge the dest with the original and updated return nodes.Dest(), nil } @@ -67,17 +67,13 @@ func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*ya return nodes.Dest(), nil } - updatedStr, err := nodes.Updated().String() + values, err := m.getStrValues(nodes) if err != nil { - return nil, errors.Wrap(err) - } - originStr, err := nodes.Origin().String() - if err != nil { - return nil, errors.Wrap(err) + return nil, err } - if updatedStr != originStr { - // change in update node + if (values.Dest == "" || values.Dest == values.Origin) && values.Origin != values.Update { + // if local is nil or is unchanged but there is new update return nodes.Updated(), nil } diff --git a/kyaml/yaml/types.go b/kyaml/yaml/types.go index 75102cdbe..df4c9ea4b 100644 --- a/kyaml/yaml/types.go +++ b/kyaml/yaml/types.go @@ -55,7 +55,6 @@ func IsYNodeEmptyMap(n *yaml.Node) bool { return n != nil && n.Kind == yaml.MappingNode && len(n.Content) == 0 } - // IsYNodeEmptyMap is true if the Node is a non-nil empty sequence. func IsYNodeEmptySeq(n *yaml.Node) bool { return n != nil && n.Kind == yaml.SequenceNode && len(n.Content) == 0 @@ -335,6 +334,13 @@ type ResourceIdentifier struct { Kind string `yaml:"kind,omitempty"` } +// Comments struct is comment yaml comment types +type Comments struct { + LineComment string `yaml:"lineComment,omitempty"` + HeadComment string `yaml:"headComment,omitempty"` + FootComment string `yaml:"footComment,omitempty"` +} + func (r *ResourceIdentifier) GetName() string { return r.Name } diff --git a/kyaml/yaml/walk/map.go b/kyaml/yaml/walk/map.go index c2fb84e32..4c40885e5 100644 --- a/kyaml/yaml/walk/map.go +++ b/kyaml/yaml/walk/map.go @@ -27,11 +27,12 @@ func (l Walker) walkMap() (*yaml.RNode, error) { // recursively set the field values on the map for _, key := range l.fieldNames() { + var res *yaml.RNode + var keys []*yaml.RNode if l.VisitKeysAsScalars { // visit the map keys as if they were scalars, // this is necessary if doing things such as copying // comments - var keys []*yaml.RNode for i := range l.Sources { // construct the sources from the keys if l.Sources[i] == nil { @@ -47,7 +48,7 @@ func (l Walker) walkMap() (*yaml.RNode, error) { } // visit the sources as a scalar // keys don't have any schema --pass in nil - _, err := l.Visitor.VisitScalar(keys, nil) + res, err = l.Visitor.VisitScalar(keys, nil) if err != nil { return nil, err } @@ -72,8 +73,23 @@ func (l Walker) walkMap() (*yaml.RNode, error) { return nil, err } + // transfer the comments of res to dest node + var comments yaml.Comments + if !yaml.IsMissingOrNull(res) { + comments = yaml.Comments{ + LineComment: res.YNode().LineComment, + HeadComment: res.YNode().HeadComment, + FootComment: res.YNode().FootComment, + } + if len(keys) > 0 && !yaml.IsMissingOrNull(keys[DestIndex]) { + keys[DestIndex].YNode().HeadComment = res.YNode().HeadComment + keys[DestIndex].YNode().LineComment = res.YNode().LineComment + keys[DestIndex].YNode().FootComment = res.YNode().FootComment + } + } + // this handles empty and non-empty values - _, err = dest.Pipe(yaml.FieldSetter{Name: key, Value: val}) + _, err = dest.Pipe(yaml.FieldSetter{Name: key, Comments: comments, Value: val}) if err != nil { return nil, err }