From 2cac624c905841407d318a19991b518fa714a36a Mon Sep 17 00:00:00 2001 From: tar Date: Tue, 26 May 2026 13:15:06 +0200 Subject: [PATCH 1/5] fix 3merge when null value not changed Signed-off-by: tar --- pkg/lib/update/merge3/tuple.go | 3 +- pkg/lib/update/merge3/visitor.go | 187 +++++++++++++++++++++++++++++++ 2 files changed, 188 insertions(+), 2 deletions(-) create mode 100644 pkg/lib/update/merge3/visitor.go diff --git a/pkg/lib/update/merge3/tuple.go b/pkg/lib/update/merge3/tuple.go index de8e39565c..f455d776e6 100644 --- a/pkg/lib/update/merge3/tuple.go +++ b/pkg/lib/update/merge3/tuple.go @@ -20,7 +20,6 @@ import ( "sigs.k8s.io/kustomize/kyaml/kio/filters" "sigs.k8s.io/kustomize/kyaml/kio/kioutil" "sigs.k8s.io/kustomize/kyaml/yaml" - "sigs.k8s.io/kustomize/kyaml/yaml/merge3" "sigs.k8s.io/kustomize/kyaml/yaml/walk" ) @@ -35,7 +34,7 @@ type tuple struct { func (t *tuple) merge() (*yaml.RNode, error) { return walk.Walker{ // same as in merge3.Merge() - Visitor: merge3.Visitor{}, + Visitor: Visitor{}, VisitKeysAsScalars: true, Sources: []*yaml.RNode{t.dest, t.original, t.updated}, diff --git a/pkg/lib/update/merge3/visitor.go b/pkg/lib/update/merge3/visitor.go new file mode 100644 index 0000000000..a7fdfe84e8 --- /dev/null +++ b/pkg/lib/update/merge3/visitor.go @@ -0,0 +1,187 @@ +// Copyright 2026 The kpt Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package merge3 + +import ( + "sigs.k8s.io/kustomize/kyaml/openapi" + "sigs.k8s.io/kustomize/kyaml/yaml" + "sigs.k8s.io/kustomize/kyaml/yaml/walk" +) + +type ConflictStrategy uint + +const ( + // TODO: Support more strategies + TakeUpdate ConflictStrategy = 1 + iota +) + +type Visitor struct{} + +func (m Visitor) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { + // explicitly cleared from either dest or update + return walk.ClearNode, nil + } + if nodes.Dest() == nil && nodes.Updated() == nil { + // implicitly cleared missing from both dest and update + return walk.ClearNode, nil + } + + if nodes.Dest() == nil { + // not cleared, but missing from the dest + // 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 +} + +func (m Visitor) visitAList(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { + if yaml.IsMissingOrNull(nodes.Updated()) && !yaml.IsMissingOrNull(nodes.Origin()) { + // implicitly cleared from update -- element was deleted + return walk.ClearNode, nil + } + if yaml.IsMissingOrNull(nodes.Dest()) { + // not cleared, but missing from the dest + // initialize a new value that can be recursively merged + return yaml.NewRNode(&yaml.Node{Kind: yaml.SequenceNode}), nil + } + + // recursively merge the dest with the original and updated + return nodes.Dest(), nil +} + +func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { + // explicitly cleared from either dest or update + return nil, nil + } + if yaml.IsMissingOrNull(nodes.Updated()) != yaml.IsMissingOrNull(nodes.Origin()) { + // value added or removed in update + return nodes.Updated(), nil + } + if yaml.IsMissingOrNull(nodes.Updated()) && yaml.IsMissingOrNull(nodes.Origin()) { + // value added or removed in update + return nodes.Dest(), nil + } + + values, err := m.getStrValues(nodes) + if err != nil { + return nil, err + } + + 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 + } + + if nodes.Updated().YNode().Value != nodes.Origin().YNode().Value { + // value changed in update + return nodes.Updated(), nil + } + + // unchanged between origin and update, keep the dest + return nodes.Dest(), nil +} + +func (m Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { + // explicitly cleared from either dest or update + return walk.ClearNode, nil + } + + if yaml.IsMissingOrNull(nodes.Updated()) != yaml.IsMissingOrNull(nodes.Origin()) { + // value added or removed in update + return nodes.Updated(), nil + } + if yaml.IsMissingOrNull(nodes.Updated()) && yaml.IsMissingOrNull(nodes.Origin()) { + // value not present in source or dest + return nodes.Dest(), nil + } + + // compare origin and update values to see if they have changed + values, err := m.getStrValues(nodes) + if err != nil { + return nil, err + } + if values.Update != values.Origin { + // value changed in update + return nodes.Updated(), nil + } + + // unchanged between origin and update, keep the dest + return nodes.Dest(), nil +} + +func (m Visitor) isCleared(origin, current *yaml.RNode) bool { + return !origin.IsTaggedNull() && current.IsTaggedNull() +} + +func (m Visitor) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, kind walk.ListKind) (*yaml.RNode, error) { + if kind == walk.AssociativeList { + return m.visitAList(nodes, s) + } + // non-associative list + return m.visitNAList(nodes) +} + +func (m Visitor) getStrValues(nodes walk.Sources) (strValues, error) { + var uStr, oStr, dStr string + var err error + if nodes.Updated() != nil && nodes.Updated().YNode() != nil { + s := nodes.Updated().YNode().Style + defer func() { + nodes.Updated().YNode().Style = s + }() + nodes.Updated().YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle + uStr, err = nodes.Updated().String() + if err != nil { + return strValues{}, err + } + } + if nodes.Origin() != nil && nodes.Origin().YNode() != nil { + s := nodes.Origin().YNode().Style + defer func() { + nodes.Origin().YNode().Style = s + }() + nodes.Origin().YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle + oStr, err = nodes.Origin().String() + if err != nil { + return strValues{}, err + } + } + if nodes.Dest() != nil && nodes.Dest().YNode() != nil { + s := nodes.Dest().YNode().Style + defer func() { + nodes.Dest().YNode().Style = s + }() + nodes.Dest().YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle + dStr, err = nodes.Dest().String() + if err != nil { + return strValues{}, err + } + } + + return strValues{Origin: oStr, Update: uStr, Dest: dStr}, nil +} + +type strValues struct { + Origin string + Update string + Dest string +} + +var _ walk.Visitor = Visitor{} From 70f20990efdb6f70b400774220af691e551b4053 Mon Sep 17 00:00:00 2001 From: tar Date: Mon, 1 Jun 2026 15:46:30 +0200 Subject: [PATCH 2/5] comment Signed-off-by: tar --- pkg/lib/update/merge3/visitor.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/lib/update/merge3/visitor.go b/pkg/lib/update/merge3/visitor.go index a7fdfe84e8..245dd7efc7 100644 --- a/pkg/lib/update/merge3/visitor.go +++ b/pkg/lib/update/merge3/visitor.go @@ -74,7 +74,7 @@ func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*ya return nodes.Updated(), nil } if yaml.IsMissingOrNull(nodes.Updated()) && yaml.IsMissingOrNull(nodes.Origin()) { - // value added or removed in update + // value absent in both origin and update return nodes.Dest(), nil } From 183f203ecbd5e61893aae706f010c8ee57eb0bda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B3zes=20L=C3=A1szl=C3=B3=20M=C3=A1t=C3=A9?= Date: Thu, 4 Jun 2026 17:25:10 +0200 Subject: [PATCH 3/5] add unit test with specific use-cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Mózes László Máté --- pkg/lib/update/common.go | 4 + pkg/lib/update/merge3/tuple.go | 2 +- pkg/lib/update/merge3/visitor.go | 75 +++++++------------ pkg/lib/update/resource-merge_test.go | 43 +++++++++++ .../testdata/keep-tagged-null/dest/Kptfile | 21 ++++++ .../keep-tagged-null/dest/configmap.yaml | 11 +++ .../keep-tagged-null/expected/Kptfile | 21 ++++++ .../keep-tagged-null/expected/configmap.yaml | 12 +++ .../testdata/keep-tagged-null/origin/Kptfile | 8 ++ .../keep-tagged-null/origin/configmap.yaml | 11 +++ .../testdata/keep-tagged-null/updated/Kptfile | 8 ++ .../keep-tagged-null/updated/configmap.yaml | 12 +++ 12 files changed, 181 insertions(+), 47 deletions(-) create mode 100644 pkg/lib/update/testdata/keep-tagged-null/dest/Kptfile create mode 100644 pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml create mode 100644 pkg/lib/update/testdata/keep-tagged-null/expected/Kptfile create mode 100644 pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml create mode 100644 pkg/lib/update/testdata/keep-tagged-null/origin/Kptfile create mode 100644 pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml create mode 100644 pkg/lib/update/testdata/keep-tagged-null/updated/Kptfile create mode 100644 pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml diff --git a/pkg/lib/update/common.go b/pkg/lib/update/common.go index 0de48039e2..83087005fe 100644 --- a/pkg/lib/update/common.go +++ b/pkg/lib/update/common.go @@ -37,6 +37,10 @@ func PkgHasUpdatedUpstream(local, origin string) (bool, error) { return false, errors.E(op, types.UniquePath(local), err) } + if originKf.Upstream == nil { + return false, nil + } + // If the upstream information in local has changed from origin, it // means the user had updated the package independently and we don't // want to override it. diff --git a/pkg/lib/update/merge3/tuple.go b/pkg/lib/update/merge3/tuple.go index f455d776e6..cb33d47f7f 100644 --- a/pkg/lib/update/merge3/tuple.go +++ b/pkg/lib/update/merge3/tuple.go @@ -34,7 +34,7 @@ type tuple struct { func (t *tuple) merge() (*yaml.RNode, error) { return walk.Walker{ // same as in merge3.Merge() - Visitor: Visitor{}, + Visitor: &Visitor{}, VisitKeysAsScalars: true, Sources: []*yaml.RNode{t.dest, t.original, t.updated}, diff --git a/pkg/lib/update/merge3/visitor.go b/pkg/lib/update/merge3/visitor.go index 245dd7efc7..8b2f4a6d8f 100644 --- a/pkg/lib/update/merge3/visitor.go +++ b/pkg/lib/update/merge3/visitor.go @@ -1,3 +1,4 @@ +// Copyright 2019 The Kubernetes Authors. // Copyright 2026 The kpt Authors // // Licensed under the Apache License, Version 2.0 (the "License"); @@ -12,25 +13,23 @@ // See the License for the specific language governing permissions and // limitations under the License. +// copied from sigs.k8s.io/kustomize/kyaml@v0.21.1/yaml/merge3/visitor.go with modifications + package merge3 import ( "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" + "sigs.k8s.io/kustomize/kyaml/yaml/merge3" "sigs.k8s.io/kustomize/kyaml/yaml/walk" ) -type ConflictStrategy uint - -const ( - // TODO: Support more strategies - TakeUpdate ConflictStrategy = 1 + iota -) - -type Visitor struct{} +type Visitor struct { + *merge3.Visitor +} -func (m Visitor) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { - if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { +func (m *Visitor) VisitMap(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { // MODIFIED // explicitly cleared from either dest or update return walk.ClearNode, nil } @@ -49,7 +48,7 @@ func (m Visitor) VisitMap(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml. return nodes.Dest(), nil } -func (m Visitor) visitAList(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { +func (m *Visitor) visitAList(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { if yaml.IsMissingOrNull(nodes.Updated()) && !yaml.IsMissingOrNull(nodes.Origin()) { // implicitly cleared from update -- element was deleted return walk.ClearNode, nil @@ -64,8 +63,8 @@ func (m Visitor) visitAList(nodes walk.Sources, _ *openapi.ResourceSchema) (*yam return nodes.Dest(), nil } -func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*yaml.RNode, error) { - if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { +func (m *Visitor) VisitScalar(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { // MODIFIED // explicitly cleared from either dest or update return nil, nil } @@ -97,8 +96,8 @@ func (m Visitor) VisitScalar(nodes walk.Sources, s *openapi.ResourceSchema) (*ya return nodes.Dest(), nil } -func (m Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { - if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { +func (m *Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { + if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { // MODIFIED // explicitly cleared from either dest or update return walk.ClearNode, nil } @@ -126,11 +125,13 @@ func (m Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { return nodes.Dest(), nil } -func (m Visitor) isCleared(origin, current *yaml.RNode) bool { - return !origin.IsTaggedNull() && current.IsTaggedNull() +// NEW +// isCleared returns if the node has been present in `left` but explicitly removed in `right` +func (*Visitor) isCleared(left, right *yaml.RNode) bool { + return !left.IsTaggedNull() && right.IsTaggedNull() } -func (m Visitor) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, 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, s) } @@ -138,38 +139,20 @@ func (m Visitor) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, kind w return m.visitNAList(nodes) } -func (m Visitor) getStrValues(nodes walk.Sources) (strValues, error) { +// SIMPLIFIED +func (m *Visitor) getStrValues(nodes walk.Sources) (strValues, error) { var uStr, oStr, dStr string var err error - if nodes.Updated() != nil && nodes.Updated().YNode() != nil { - s := nodes.Updated().YNode().Style - defer func() { - nodes.Updated().YNode().Style = s - }() - nodes.Updated().YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle - uStr, err = nodes.Updated().String() - if err != nil { - return strValues{}, err - } - } - if nodes.Origin() != nil && nodes.Origin().YNode() != nil { - s := nodes.Origin().YNode().Style - defer func() { - nodes.Origin().YNode().Style = s - }() - nodes.Origin().YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle - oStr, err = nodes.Origin().String() - if err != nil { - return strValues{}, err + for _, rnode := range []*yaml.RNode{nodes.Updated(), nodes.Origin(), nodes.Dest()} { + if rnode == nil || rnode.YNode() == nil { + continue } - } - if nodes.Dest() != nil && nodes.Dest().YNode() != nil { - s := nodes.Dest().YNode().Style + s := rnode.YNode().Style defer func() { - nodes.Dest().YNode().Style = s + rnode.YNode().Style = s }() - nodes.Dest().YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle - dStr, err = nodes.Dest().String() + rnode.YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle + uStr, err = rnode.String() if err != nil { return strValues{}, err } @@ -184,4 +167,4 @@ type strValues struct { Dest string } -var _ walk.Visitor = Visitor{} +var _ walk.Visitor = &Visitor{} diff --git a/pkg/lib/update/resource-merge_test.go b/pkg/lib/update/resource-merge_test.go index b68072d607..05e4eb07f3 100644 --- a/pkg/lib/update/resource-merge_test.go +++ b/pkg/lib/update/resource-merge_test.go @@ -16,14 +16,17 @@ package update_test import ( + "os" "path/filepath" "testing" + "github.com/google/go-cmp/cmp" "github.com/kptdev/kpt/internal/testutil" "github.com/kptdev/kpt/internal/testutil/pkgbuilder" "github.com/kptdev/kpt/pkg/lib/update" "github.com/kptdev/kpt/pkg/lib/update/updatetypes" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestUpdate_ResourceMerge(t *testing.T) { @@ -301,3 +304,43 @@ func TestUpdate_ResourceMerge(t *testing.T) { }) } } + +func TestKeepTaggedNull(t *testing.T) { + updater := update.ResourceMergeUpdater{} + + testdata, err := filepath.Abs(filepath.Join(".", "testdata", "keep-tagged-null")) + require.NoError(t, err) + + tmpDir := t.TempDir() + newDest := filepath.Join(tmpDir, "dest") + err = os.MkdirAll(newDest, 0755) + require.NoError(t, err) + + oldDestFiles, err := os.ReadDir(filepath.Join(testdata, "dest")) + require.NoError(t, err) + for _, file := range oldDestFiles { + if file.Type().IsRegular() { + content, err := os.ReadFile(filepath.Join(testdata, "dest", file.Name())) + require.NoError(t, err) + err = os.WriteFile(filepath.Join(newDest, file.Name()), content, 0644) + require.NoError(t, err) + } + } + + options := updatetypes.Options{ + OriginPath: filepath.Join(testdata, "origin"), + UpdatedPath: filepath.Join(testdata, "updated"), + LocalPath: newDest, + } + err = updater.Update(options) + require.NoError(t, err) + + destBytes, err := os.ReadFile(filepath.Join(newDest, "configmap.yaml")) + require.NoError(t, err) + expBytes, err := os.ReadFile(filepath.Join(testdata, "expected", "configmap.yaml")) + require.NoError(t, err) + + if diff := cmp.Diff(expBytes, destBytes); diff != "" { + t.Errorf("unexpected result (-want, +got): %s", diff) + } +} diff --git a/pkg/lib/update/testdata/keep-tagged-null/dest/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/dest/Kptfile new file mode 100644 index 0000000000..068e1941c4 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/dest/Kptfile @@ -0,0 +1,21 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +upstream: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 +upstreamLock: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 + commit: b3e1d439516a5e8d49adc0c82d3e95578570dbfa +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml new file mode 100644 index 0000000000..cd8a0fd646 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml @@ -0,0 +1,11 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test +data: + replicas: 1 + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' diff --git a/pkg/lib/update/testdata/keep-tagged-null/expected/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/expected/Kptfile new file mode 100644 index 0000000000..068e1941c4 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/expected/Kptfile @@ -0,0 +1,21 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +upstream: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 +upstreamLock: + type: git + git: + repo: https://github.com/droot/pkg-catalog.git + directory: basens + ref: basens/v0 + commit: b3e1d439516a5e8d49adc0c82d3e95578570dbfa +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml new file mode 100644 index 0000000000..c28258422c --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml @@ -0,0 +1,12 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test +data: + replicas: 1 + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: "something" + emptyStringKey2: 'something' + missKey: something diff --git a/pkg/lib/update/testdata/keep-tagged-null/origin/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/origin/Kptfile new file mode 100644 index 0000000000..7857f395c5 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/origin/Kptfile @@ -0,0 +1,8 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml new file mode 100644 index 0000000000..cd8a0fd646 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml @@ -0,0 +1,11 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test +data: + replicas: 1 + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' diff --git a/pkg/lib/update/testdata/keep-tagged-null/updated/Kptfile b/pkg/lib/update/testdata/keep-tagged-null/updated/Kptfile new file mode 100644 index 0000000000..7857f395c5 --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/updated/Kptfile @@ -0,0 +1,8 @@ +apiVersion: kpt.dev/v1 +kind: Kptfile +metadata: + name: test + annotations: + config.kubernetes.io/local-config: "true" +info: + description: Test package. diff --git a/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml new file mode 100644 index 0000000000..e73834873c --- /dev/null +++ b/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml @@ -0,0 +1,12 @@ +apiVersion: v1 +kind: ConfigMap +metadata: + name: test +data: + replicas: 1 + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: something + emptyStringKey2: something + missKey: something From 7a3a64ae3b0f8b0e93ee62e40259b8a8118bcbea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=B3zes=20L=C3=A1szl=C3=B3=20M=C3=A1t=C3=A9?= Date: Thu, 4 Jun 2026 18:03:23 +0200 Subject: [PATCH 4/5] fix getStrValues MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Mózes László Máté --- pkg/lib/update/merge3/visitor.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/pkg/lib/update/merge3/visitor.go b/pkg/lib/update/merge3/visitor.go index 8b2f4a6d8f..a9fe3dd420 100644 --- a/pkg/lib/update/merge3/visitor.go +++ b/pkg/lib/update/merge3/visitor.go @@ -143,7 +143,16 @@ func (m *Visitor) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, kind func (m *Visitor) getStrValues(nodes walk.Sources) (strValues, error) { var uStr, oStr, dStr string var err error - for _, rnode := range []*yaml.RNode{nodes.Updated(), nodes.Origin(), nodes.Dest()} { + + for _, p := range []struct { + rnode *yaml.RNode + str *string + }{ + {nodes.Updated(), &uStr}, + {nodes.Origin(), &oStr}, + {nodes.Dest(), &dStr}, + } { + rnode := p.rnode if rnode == nil || rnode.YNode() == nil { continue } @@ -152,7 +161,7 @@ func (m *Visitor) getStrValues(nodes walk.Sources) (strValues, error) { rnode.YNode().Style = s }() rnode.YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle - uStr, err = rnode.String() + *p.str, err = rnode.String() if err != nil { return strValues{}, err } From 23f82eb76d41360fc3342d55cb8985a0bc8bedf4 Mon Sep 17 00:00:00 2001 From: tar Date: Fri, 5 Jun 2026 07:05:51 +0200 Subject: [PATCH 5/5] eliminate stringify, more tests Signed-off-by: tar --- pkg/lib/update/merge3/merge3_test.go | 14 ++++ pkg/lib/update/merge3/merge3_util_test.go | 8 +++ .../testdata/non-assoc-crd/destination.yaml | 32 +++++++++ .../testdata/non-assoc-crd/original.yaml | 30 ++++++++ .../testdata/non-assoc-crd/updated.yaml | 32 +++++++++ pkg/lib/update/merge3/tuple.go | 4 +- pkg/lib/update/merge3/visitor.go | 72 +++++-------------- .../keep-tagged-null/dest/configmap.yaml | 23 ++++-- .../keep-tagged-null/expected/configmap.yaml | 23 +++--- .../keep-tagged-null/origin/configmap.yaml | 24 +++++-- .../keep-tagged-null/updated/configmap.yaml | 26 ++++--- 11 files changed, 204 insertions(+), 84 deletions(-) create mode 100644 pkg/lib/update/merge3/testdata/non-assoc-crd/destination.yaml create mode 100644 pkg/lib/update/merge3/testdata/non-assoc-crd/original.yaml create mode 100644 pkg/lib/update/merge3/testdata/non-assoc-crd/updated.yaml diff --git a/pkg/lib/update/merge3/merge3_test.go b/pkg/lib/update/merge3/merge3_test.go index e71cde2259..e0fef7e41e 100644 --- a/pkg/lib/update/merge3/merge3_test.go +++ b/pkg/lib/update/merge3/merge3_test.go @@ -158,3 +158,17 @@ func (t *Merge3TestSuite) TestInferAssocList() { t.commonTest(name, tc) } } + +func (t *Merge3TestSuite) TestNonAssocList() { + testCases := map[string]testCase{ + "non-assoc-crd": { + dir: "non-assoc-crd", + crds: []string{}, + checkFn: nonAssocListMergeCheck, + }, + } + + for name, tc := range testCases { + t.commonTest(name, tc) + } +} diff --git a/pkg/lib/update/merge3/merge3_util_test.go b/pkg/lib/update/merge3/merge3_util_test.go index c8901d2487..765767adf8 100644 --- a/pkg/lib/update/merge3/merge3_util_test.go +++ b/pkg/lib/update/merge3/merge3_util_test.go @@ -164,6 +164,14 @@ func assocListMergeCheck(t *Merge3TestSuite, kos fn.KubeObjects) { })(t, kos) } +func nonAssocListMergeCheck(t *Merge3TestSuite, kos fn.KubeObjects) { + makeFruitCheckFunc(10, map[string]int{ + "apple": 20, + "grape": 5, + "pear": 30, + })(t, kos) +} + //nolint:unparam func makeFruitCheckFunc(expectedTemp int, expectedFruits map[string]int) func(*Merge3TestSuite, fn.KubeObjects) { return func(t *Merge3TestSuite, kos fn.KubeObjects) { diff --git a/pkg/lib/update/merge3/testdata/non-assoc-crd/destination.yaml b/pkg/lib/update/merge3/testdata/non-assoc-crd/destination.yaml new file mode 100644 index 0000000000..c7ed900e51 --- /dev/null +++ b/pkg/lib/update/merge3/testdata/non-assoc-crd/destination.yaml @@ -0,0 +1,32 @@ +apiVersion: infer.kpt.dev/v1alpha1 +kind: MergeTestResources +metadata: + name: "test-resources" +spec: + packageName: "test-package" + workspaceName: "v2" + revision: 2 + repositoryName: "test-repo" + resources: + Kptfile: | + apiVersion: kpt.dev/v1 + kind: Kptfile + metadata: + name: test-package + info: + description: test package + fruitstore.yaml: | + apiVersion: test.group/v1 + kind: FruitStore + metadata: + name: test-fruit-store + spec: + airConditioned: false + preferredTemperature: 20 + fruits: + - name: grape + amount: 5 + - name: apple + amount: 25 + - name: banana + amount: 3 diff --git a/pkg/lib/update/merge3/testdata/non-assoc-crd/original.yaml b/pkg/lib/update/merge3/testdata/non-assoc-crd/original.yaml new file mode 100644 index 0000000000..7dc5440c2f --- /dev/null +++ b/pkg/lib/update/merge3/testdata/non-assoc-crd/original.yaml @@ -0,0 +1,30 @@ +apiVersion: infer.kpt.dev/v1alpha1 +kind: MergeTestResources +metadata: + name: "test-resources" +spec: + packageName: "test-package" + workspaceName: "v1" + revision: 1 + repositoryName: "test-repo" + resources: + Kptfile: | + apiVersion: kpt.dev/v1 + kind: Kptfile + metadata: + name: test-package + info: + description: test package + fruitstore.yaml: | + apiVersion: test.group/v1 + kind: FruitStore + metadata: + name: test-fruit-store + spec: + airConditioned: false + preferredTemperature: 15 + fruits: + - name: apple + amount: 10 + - name: grape + amount: 5 diff --git a/pkg/lib/update/merge3/testdata/non-assoc-crd/updated.yaml b/pkg/lib/update/merge3/testdata/non-assoc-crd/updated.yaml new file mode 100644 index 0000000000..c3592af0c0 --- /dev/null +++ b/pkg/lib/update/merge3/testdata/non-assoc-crd/updated.yaml @@ -0,0 +1,32 @@ +apiVersion: infer.kpt.dev/v1alpha1 +kind: MergeTestResources +metadata: + name: "test-resources" +spec: + packageName: "test-package" + workspaceName: "v3" + revision: 3 + repositoryName: "test-repo" + resources: + Kptfile: | + apiVersion: kpt.dev/v1 + kind: Kptfile + metadata: + name: test-package + info: + description: test package + fruitstore.yaml: | + apiVersion: test.group/v1 + kind: FruitStore + metadata: + name: test-fruit-store + spec: + airConditioned: false + preferredTemperature: 10 + fruits: + - name: pear + amount: 30 + - name: apple + amount: 20 + - name: grape + amount: 5 diff --git a/pkg/lib/update/merge3/tuple.go b/pkg/lib/update/merge3/tuple.go index cb33d47f7f..5b7af366fb 100644 --- a/pkg/lib/update/merge3/tuple.go +++ b/pkg/lib/update/merge3/tuple.go @@ -33,8 +33,10 @@ type tuple struct { // merge performs a 3-way merge on the tuple func (t *tuple) merge() (*yaml.RNode, error) { return walk.Walker{ + // modified Visitor + Visitor: &Visitor{}, + // same as in merge3.Merge() - Visitor: &Visitor{}, VisitKeysAsScalars: true, Sources: []*yaml.RNode{t.dest, t.original, t.updated}, diff --git a/pkg/lib/update/merge3/visitor.go b/pkg/lib/update/merge3/visitor.go index a9fe3dd420..2ffc80fb1b 100644 --- a/pkg/lib/update/merge3/visitor.go +++ b/pkg/lib/update/merge3/visitor.go @@ -20,13 +20,10 @@ package merge3 import ( "sigs.k8s.io/kustomize/kyaml/openapi" "sigs.k8s.io/kustomize/kyaml/yaml" - "sigs.k8s.io/kustomize/kyaml/yaml/merge3" "sigs.k8s.io/kustomize/kyaml/yaml/walk" ) -type Visitor struct { - *merge3.Visitor -} +type Visitor struct{} func (m *Visitor) VisitMap(nodes walk.Sources, _ *openapi.ResourceSchema) (*yaml.RNode, error) { if m.isCleared(nodes.Origin(), nodes.Updated()) || m.isCleared(nodes.Origin(), nodes.Dest()) { // MODIFIED @@ -77,16 +74,6 @@ func (m *Visitor) VisitScalar(nodes walk.Sources, _ *openapi.ResourceSchema) (*y return nodes.Dest(), nil } - values, err := m.getStrValues(nodes) - if err != nil { - return nil, err - } - - 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 - } - if nodes.Updated().YNode().Value != nodes.Origin().YNode().Value { // value changed in update return nodes.Updated(), nil @@ -111,12 +98,7 @@ func (m *Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { return nodes.Dest(), nil } - // compare origin and update values to see if they have changed - values, err := m.getStrValues(nodes) - if err != nil { - return nil, err - } - if values.Update != values.Origin { + if !m.isNodeContentEqual(nodes.Origin().YNode(), nodes.Updated().YNode()) { // value changed in update return nodes.Updated(), nil } @@ -126,7 +108,7 @@ func (m *Visitor) visitNAList(nodes walk.Sources) (*yaml.RNode, error) { } // NEW -// isCleared returns if the node has been present in `left` but explicitly removed in `right` +// isCleared returns if the node has not tagged null in `left` but explicitly removed in `right` func (*Visitor) isCleared(left, right *yaml.RNode) bool { return !left.IsTaggedNull() && right.IsTaggedNull() } @@ -140,40 +122,24 @@ func (m *Visitor) VisitList(nodes walk.Sources, s *openapi.ResourceSchema, kind } // SIMPLIFIED -func (m *Visitor) getStrValues(nodes walk.Sources) (strValues, error) { - var uStr, oStr, dStr string - var err error - - for _, p := range []struct { - rnode *yaml.RNode - str *string - }{ - {nodes.Updated(), &uStr}, - {nodes.Origin(), &oStr}, - {nodes.Dest(), &dStr}, - } { - rnode := p.rnode - if rnode == nil || rnode.YNode() == nil { - continue - } - s := rnode.YNode().Style - defer func() { - rnode.YNode().Style = s - }() - rnode.YNode().Style = yaml.FlowStyle | yaml.SingleQuotedStyle - *p.str, err = rnode.String() - if err != nil { - return strValues{}, err +// isNodeContentEqual compares the nodes structurally (kind, tag, value and children), +// avoiding YAML serialization. Presentation style and comments are ignored. +func (m *Visitor) isNodeContentEqual(a, b *yaml.Node) bool { + if a == nil || b == nil { + return a == b + } + if a.Kind != b.Kind || a.Tag != b.Tag || a.Value != b.Value { + return false + } + if len(a.Content) != len(b.Content) { + return false + } + for i := range a.Content { + if !m.isNodeContentEqual(a.Content[i], b.Content[i]) { + return false } } - - return strValues{Origin: oStr, Update: uStr, Dest: dStr}, nil -} - -type strValues struct { - Origin string - Update string - Dest string + return true } var _ walk.Visitor = &Visitor{} diff --git a/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml index cd8a0fd646..73b6cea9ac 100644 --- a/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml +++ b/pkg/lib/update/testdata/keep-tagged-null/dest/configmap.yaml @@ -1,11 +1,20 @@ apiVersion: v1 kind: ConfigMap metadata: - name: test + name: m7 + namespace: default data: - replicas: 1 - nullKey: null - unsetKey: - tildeKey: ~ - emptyStringKey1: "" - emptyStringKey2: '' + deletedInOrigin: + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' + deletedInDest: + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' + addedInUpdated: + unchange: origin diff --git a/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml index c28258422c..b0f4b84153 100644 --- a/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml +++ b/pkg/lib/update/testdata/keep-tagged-null/expected/configmap.yaml @@ -1,12 +1,19 @@ apiVersion: v1 kind: ConfigMap metadata: - name: test + name: m7 + namespace: default data: - replicas: 1 - nullKey: something - unsetKey: something - tildeKey: something - emptyStringKey1: "something" - emptyStringKey2: 'something' - missKey: something + deletedInOrigin: + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: "something" + emptyStringKey2: 'something' + deletedInDest: + emptyStringKey1: "something" + emptyStringKey2: 'something' + missKey: something + addedInUpdated: + unchange: origin + addedKey: something diff --git a/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml index cd8a0fd646..96b5244204 100644 --- a/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml +++ b/pkg/lib/update/testdata/keep-tagged-null/origin/configmap.yaml @@ -1,11 +1,21 @@ apiVersion: v1 kind: ConfigMap metadata: - name: test + name: m7 + namespace: default data: - replicas: 1 - nullKey: null - unsetKey: - tildeKey: ~ - emptyStringKey1: "" - emptyStringKey2: '' + deletedInOrigin: + nullKey: null + unsetKey: + tildeKey: ~ + emptyStringKey1: "" + emptyStringKey2: '' + deletedInDest: + nullKey: origin + unsetKey: origin + tildeKey: origin + emptyStringKey1: origin + emptyStringKey2: origin + missKey: origin + addedInUpdated: + unchange: origin \ No newline at end of file diff --git a/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml b/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml index e73834873c..ccfdcd36ed 100644 --- a/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml +++ b/pkg/lib/update/testdata/keep-tagged-null/updated/configmap.yaml @@ -1,12 +1,22 @@ apiVersion: v1 kind: ConfigMap metadata: - name: test + name: m7 + namespace: default data: - replicas: 1 - nullKey: something - unsetKey: something - tildeKey: something - emptyStringKey1: something - emptyStringKey2: something - missKey: something + deletedInOrigin: + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: something + emptyStringKey2: something + deletedInDest: + nullKey: something + unsetKey: something + tildeKey: something + emptyStringKey1: something + emptyStringKey2: something + missKey: something + addedInUpdated: + unchange: origin + addedKey: something \ No newline at end of file