diff --git a/api/builtins/PatchStrategicMergeTransformer.go b/api/builtins/PatchStrategicMergeTransformer.go index 1f53c5dab5..f93d1266d7 100644 --- a/api/builtins/PatchStrategicMergeTransformer.go +++ b/api/builtins/PatchStrategicMergeTransformer.go @@ -45,15 +45,6 @@ func (p *PatchStrategicMergeTransformerPlugin) Config( return fmt.Errorf( "patch appears to be empty; files=%v, Patch=%s", p.Paths, p.Patches) } - // TODO(#3723): Delete conflict detection. - // Since #1500 closed, the conflict detector in use doesn't do - // anything useful. The resmap returned by this method hasn't - // been used for many releases. Leaving code as a comment to - // aid in deletion (fixing #3723). - // _, err = h.ResmapFactory().ConflatePatches(p.loadedPatches) - // if err != nil { - // return err - // } return nil } diff --git a/api/filters/nameref/nameref_test.go b/api/filters/nameref/nameref_test.go index 8ddc2aa845..cc7e5de845 100644 --- a/api/filters/nameref/nameref_test.go +++ b/api/filters/nameref/nameref_test.go @@ -233,7 +233,7 @@ map: } tc.filter.Referrer = referrer - resMapFactory := resmap.NewFactory(factory, nil) + resMapFactory := resmap.NewFactory(factory) candidatesRes, err := factory.SliceFromBytesWithNames( tc.originalNames, []byte(tc.candidates)) if err != nil { @@ -334,7 +334,7 @@ metadata: } tc.filter.Referrer = referrer - resMapFactory := resmap.NewFactory(factory, nil) + resMapFactory := resmap.NewFactory(factory) candidatesRes, err := factory.SliceFromBytesWithNames( tc.originalNames, []byte(tc.candidates)) if err != nil { @@ -751,7 +751,7 @@ ref: } tc.filter.Referrer = referrer - resMapFactory := resmap.NewFactory(factory, nil) + resMapFactory := resmap.NewFactory(factory) candidatesRes, err := factory.SliceFromBytesWithNames( tc.originalNames, []byte(tc.candidates)) if err != nil { diff --git a/api/internal/conflict/factory.go b/api/internal/conflict/factory.go deleted file mode 100644 index 3cfed193a1..0000000000 --- a/api/internal/conflict/factory.go +++ /dev/null @@ -1,23 +0,0 @@ -// Copyright 2020 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package conflict - -import ( - "sigs.k8s.io/kustomize/api/resid" - "sigs.k8s.io/kustomize/api/resource" -) - -type cdFactory struct{} - -var _ resource.ConflictDetectorFactory = &cdFactory{} - -// NewFactory returns a new conflict detector factory. -func NewFactory() resource.ConflictDetectorFactory { - return &cdFactory{} -} - -// New returns an instance of smPatchMergeOnlyDetector. -func (c cdFactory) New(_ resid.Gvk) (resource.ConflictDetector, error) { - return &smPatchMergeOnlyDetector{}, nil -} diff --git a/api/internal/conflict/smpatchmergeonlydetector.go b/api/internal/conflict/smpatchmergeonlydetector.go deleted file mode 100644 index a33fdc3c38..0000000000 --- a/api/internal/conflict/smpatchmergeonlydetector.go +++ /dev/null @@ -1,33 +0,0 @@ -// Copyright 2020 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package conflict - -import ( - "sigs.k8s.io/kustomize/api/resource" -) - -// smPatchMergeOnlyDetector ignores conflicts, -// but does real strategic merge patching. -// This is part of an effort to eliminate dependence on -// apimachinery package to allow kustomize integration -// into kubectl (#2506 and #1500) -type smPatchMergeOnlyDetector struct{} - -var _ resource.ConflictDetector = &smPatchMergeOnlyDetector{} - -func (c *smPatchMergeOnlyDetector) HasConflict( - _, _ *resource.Resource) (bool, error) { - return false, nil -} - -// There's at least one case that doesn't work. Suppose one has a -// Deployment with a volume with the bizarre "emptyDir: {}" entry. -// If you want to get rid of this entry via a patch containing -// the entry "emptyDir: null", then the following won't work, -// because null entries are eliminated. -func (c *smPatchMergeOnlyDetector) MergePatches( - r, patch *resource.Resource) (*resource.Resource, error) { - err := r.ApplySmPatch(patch) - return r, err -} diff --git a/api/internal/plugins/execplugin/execplugin_test.go b/api/internal/plugins/execplugin/execplugin_test.go index f49e02d5fc..f943a2464e 100644 --- a/api/internal/plugins/execplugin/execplugin_test.go +++ b/api/internal/plugins/execplugin/execplugin_test.go @@ -32,8 +32,7 @@ s/$BAR/bar baz/g t.Fatal(err) } pvd := provider.NewDefaultDepProvider() - rf := resmap.NewFactory( - pvd.GetResourceFactory(), pvd.GetConflictDetectorFactory()) + rf := resmap.NewFactory(pvd.GetResourceFactory()) pluginConfig := rf.RF().FromMap( map[string]interface{}{ "apiVersion": "someteam.example.com/v1", diff --git a/api/internal/plugins/loader/loader_test.go b/api/internal/plugins/loader/loader_test.go index da6c16debe..4516179652 100644 --- a/api/internal/plugins/loader/loader_test.go +++ b/api/internal/plugins/loader/loader_test.go @@ -51,8 +51,7 @@ func TestLoader(t *testing.T) { BuildGoPlugin("someteam.example.com", "v1", "SomeServiceGenerator") defer th.Reset() p := provider.NewDefaultDepProvider() - rmF := resmap.NewFactory( - p.GetResourceFactory(), p.GetConflictDetectorFactory()) + rmF := resmap.NewFactory(p.GetResourceFactory()) fLdr, err := loader.NewLoader( loader.RestrictionRootOnly, filesys.Separator, filesys.MakeFsInMemory()) diff --git a/api/internal/target/maker_test.go b/api/internal/target/maker_test.go index 1a4f3c80a2..a0d0e6512d 100644 --- a/api/internal/target/maker_test.go +++ b/api/internal/target/maker_test.go @@ -36,8 +36,7 @@ func makeKustTargetWithRf( if err != nil { t.Fatal(err) } - rf := resmap.NewFactory( - pvd.GetResourceFactory(), pvd.GetConflictDetectorFactory()) + rf := resmap.NewFactory(pvd.GetResourceFactory()) pc := konfig.DisabledPluginConfig() return target.NewKustTarget( ldr, diff --git a/api/internal/target/multitransformer.go b/api/internal/target/multitransformer.go index 552e7f5d33..caf1bd2de4 100644 --- a/api/internal/target/multitransformer.go +++ b/api/internal/target/multitransformer.go @@ -4,15 +4,12 @@ package target import ( - "fmt" - "sigs.k8s.io/kustomize/api/resmap" ) // multiTransformer contains a list of transformers. type multiTransformer struct { - transformers []resmap.Transformer - checkConflictEnabled bool + transformers []resmap.Transformer } var _ resmap.Transformer = &multiTransformer{} @@ -20,8 +17,8 @@ var _ resmap.Transformer = &multiTransformer{} // newMultiTransformer constructs a multiTransformer. func newMultiTransformer(t []resmap.Transformer) resmap.Transformer { r := &multiTransformer{ - transformers: make([]resmap.Transformer, len(t)), - checkConflictEnabled: false} + transformers: make([]resmap.Transformer, len(t)), + } copy(r.transformers, t) return r } @@ -29,13 +26,6 @@ func newMultiTransformer(t []resmap.Transformer) resmap.Transformer { // Transform applies the member transformers in order to the resources, // optionally detecting and erroring on commutation conflict. func (o *multiTransformer) Transform(m resmap.ResMap) error { - if o.checkConflictEnabled { - return o.transformWithCheckConflict(m) - } - return o.transform(m) -} - -func (o *multiTransformer) transform(m resmap.ResMap) error { for _, t := range o.transformers { if err := t.Transform(m); err != nil { return err @@ -44,30 +34,3 @@ func (o *multiTransformer) transform(m resmap.ResMap) error { } return nil } - -// Of the len(o.transformers)! possible transformer orderings, compare to a reversed order. -// A spot check to perform when the transformations are supposed to be commutative. -// Fail if there's a difference in the result. -func (o *multiTransformer) transformWithCheckConflict(m resmap.ResMap) error { - mcopy := m.DeepCopy() - err := o.transform(m) - if err != nil { - return err - } - o.reverseTransformers() - err = o.transform(mcopy) - if err != nil { - return err - } - err = m.ErrorIfNotEqualSets(mcopy) - if err != nil { - return fmt.Errorf("found conflict between different patches\n%v", err) - } - return nil -} - -func (o *multiTransformer) reverseTransformers() { - for i, j := 0, len(o.transformers)-1; i < j; i, j = i+1, j-1 { - o.transformers[i], o.transformers[j] = o.transformers[j], o.transformers[i] - } -} diff --git a/api/krusty/kustomizer.go b/api/krusty/kustomizer.go index 591db8ed93..94cd651bd2 100644 --- a/api/krusty/kustomizer.go +++ b/api/krusty/kustomizer.go @@ -52,9 +52,7 @@ func MakeKustomizer(o *Options) *Kustomizer { // and Run can be called on each of them). func (b *Kustomizer) Run( fSys filesys.FileSystem, path string) (resmap.ResMap, error) { - resmapFactory := resmap.NewFactory( - b.depProvider.GetResourceFactory(), - b.depProvider.GetConflictDetectorFactory()) + resmapFactory := resmap.NewFactory(b.depProvider.GetResourceFactory()) lr := fLdr.RestrictionNone if b.options.LoadRestrictions == types.LoadRestrictionsRootOnly { lr = fLdr.RestrictionRootOnly diff --git a/api/krusty/multiplepatch_test.go b/api/krusty/multiplepatch_test.go index fa8b321a36..8c88f3d0e1 100644 --- a/api/krusty/multiplepatch_test.go +++ b/api/krusty/multiplepatch_test.go @@ -706,7 +706,7 @@ metadata: `) } -func TestMultiplePatchesWithConflict(t *testing.T) { +func TestNonCommutablePatches(t *testing.T) { th := kusttest_test.MakeHarness(t) makeCommonFilesForMultiplePatchTests(th) th.WriteF("overlay/staging/deployment-patch1.yaml", ` diff --git a/api/provider/depprovider.go b/api/provider/depprovider.go index 700b2e5670..0102c89ce0 100644 --- a/api/provider/depprovider.go +++ b/api/provider/depprovider.go @@ -6,51 +6,26 @@ package provider import ( "sigs.k8s.io/kustomize/api/hasher" "sigs.k8s.io/kustomize/api/ifc" - "sigs.k8s.io/kustomize/api/internal/conflict" "sigs.k8s.io/kustomize/api/internal/validate" "sigs.k8s.io/kustomize/api/resource" ) // DepProvider is a dependency provider, injecting different // implementations depending on the context. -// -// Notes on interfaces: -// -// - resource.ConflictDetector -// -// implemented by api/internal/conflict.smPatchMergeOnlyDetector -// -// At time of writing, this doesn't report conflicts, -// but it does know how to merge patches. Conflict -// reporting isn't vital to kustomize function. It's -// rare that a person would configure one transformer -// with many patches, much less so many that it became -// hard to spot conflicts. In the case of an undetected -// conflict, the last patch applied wins, likely what -// the user wants anyway. Regardless, the effect of this -// is plainly visible and usable in the output, even if -// a conflict happened but wasn't reported as an error. -// -// - ifc.Validator -// -// implemented by api/internal/validate.FieldValidator -// -// See TODO inside the validator for status. -// At time of writing, this is a do-nothing -// validator as it's not critical to kustomize function. -// type DepProvider struct { - resourceFactory *resource.Factory - conflictDectectorFactory resource.ConflictDetectorFactory - fieldValidator ifc.Validator + resourceFactory *resource.Factory + // implemented by api/internal/validate.FieldValidator + // See TODO inside the validator for status. + // At time of writing, this is a do-nothing + // validator as it's not critical to kustomize function. + fieldValidator ifc.Validator } func NewDepProvider() *DepProvider { rf := resource.NewFactory(&hasher.Hasher{}) return &DepProvider{ - resourceFactory: rf, - conflictDectectorFactory: conflict.NewFactory(), - fieldValidator: validate.NewFieldValidator(), + resourceFactory: rf, + fieldValidator: validate.NewFieldValidator(), } } @@ -62,10 +37,6 @@ func (dp *DepProvider) GetResourceFactory() *resource.Factory { return dp.resourceFactory } -func (dp *DepProvider) GetConflictDetectorFactory() resource.ConflictDetectorFactory { - return dp.conflictDectectorFactory -} - func (dp *DepProvider) GetFieldValidator() ifc.Validator { return dp.fieldValidator } diff --git a/api/resmap/factory.go b/api/resmap/factory.go index 0e287530c8..9ec0fe3967 100644 --- a/api/resmap/factory.go +++ b/api/resmap/factory.go @@ -16,14 +16,11 @@ import ( type Factory struct { // Makes resources. resF *resource.Factory - // Makes ConflictDetectors. - cdf resource.ConflictDetectorFactory } // NewFactory returns a new resmap.Factory. -func NewFactory( - rf *resource.Factory, cdf resource.ConflictDetectorFactory) *Factory { - return &Factory{resF: rf, cdf: cdf} +func NewFactory(rf *resource.Factory) *Factory { + return &Factory{resF: rf} } // RF returns a resource.Factory. @@ -126,13 +123,6 @@ func (rmF *Factory) FromSecretArgs( return rmF.FromResource(res), nil } -// ConflatePatches creates a new ResMap containing a merger of the -// incoming patches. -// Error if conflict found. -func (rmF *Factory) ConflatePatches(patches []*resource.Resource) (ResMap, error) { - return (&merginator{cdf: rmF.cdf}).ConflatePatches(patches) -} - func newResMapFromResourceSlice( resources []*resource.Resource) (ResMap, error) { result := New() diff --git a/api/resmap/factory_test.go b/api/resmap/factory_test.go index e0f2ff6019..515b2fee8a 100644 --- a/api/resmap/factory_test.go +++ b/api/resmap/factory_test.go @@ -13,7 +13,6 @@ import ( "sigs.k8s.io/kustomize/api/kv" "sigs.k8s.io/kustomize/api/loader" . "sigs.k8s.io/kustomize/api/resmap" - "sigs.k8s.io/kustomize/api/resource" resmaptest_test "sigs.k8s.io/kustomize/api/testutils/resmaptest" valtest_test "sigs.k8s.io/kustomize/api/testutils/valtest" "sigs.k8s.io/kustomize/api/types" @@ -350,56 +349,3 @@ metadata: }) } } - -func TestConflatePatches_Empty(t *testing.T) { - rm, err := rmF.ConflatePatches([]*resource.Resource{}) - assert.NoError(t, err) - assert.Equal(t, 0, rm.Size()) -} - -func TestConflatePatches(t *testing.T) { - var ( - err error - yml []byte - r1, r2 *resource.Resource - ) - r1, err = rf.FromBytes([]byte(`apiVersion: example.com/v1 -kind: Foo -metadata: - name: my-foo -spec: - bar: - B: - C: Z -`)) - assert.NoError(t, err) - - r2, err = rf.FromBytes([]byte(`apiVersion: example.com/v1 -kind: Foo -metadata: - name: my-foo -spec: - bar: - C: Z - D: W - baz: - hello: world -`)) - assert.NoError(t, err) - - rm, err := rmF.ConflatePatches([]*resource.Resource{r1, r2}) - assert.NoError(t, err) - yml, err = rm.AsYaml() - assert.NoError(t, err) - assert.Equal(t, `apiVersion: example.com/v1 -kind: Foo -metadata: - name: my-foo -spec: - bar: - C: Z - D: W - baz: - hello: world -`, string(yml)) -} diff --git a/api/resmap/merginator.go b/api/resmap/merginator.go deleted file mode 100644 index c09b5aacb2..0000000000 --- a/api/resmap/merginator.go +++ /dev/null @@ -1,123 +0,0 @@ -// Copyright 2020 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package resmap - -import ( - "fmt" - - "sigs.k8s.io/kustomize/api/resource" -) - -// merginator coordinates merging the resources in incoming to the result. -type merginator struct { - incoming []*resource.Resource - cdf resource.ConflictDetectorFactory - result ResMap -} - -func (m *merginator) ConflatePatches(in []*resource.Resource) (ResMap, error) { - m.result = New() - m.incoming = in - for index := range m.incoming { - alreadyInResult, err := m.appendIfNoMatch(index) - if err != nil { - return nil, err - } - if alreadyInResult != nil { - // The resource at index has the same resId as a previously - // considered resource. - // - // If they conflict with each other (e.g. they both want to change - // the image name in a Deployment, but to different values), - // return an error. - // - // If they don't conflict, then merge them into a single resource, - // since they both target the same item, and we want cumulative - // behavior. E.g. say both patches modify a map. Without a merge, - // the last patch wins, replacing the entire map. - err = m.mergeWithExisting(index, alreadyInResult) - if err != nil { - return nil, err - } - } - } - return m.result, nil -} - -func (m *merginator) appendIfNoMatch(index int) (*resource.Resource, error) { - candidate := m.incoming[index] - matchedResources := m.result.GetMatchingResourcesByAnyId( - candidate.OrgId().Equals) - if len(matchedResources) == 0 { - m.result.Append(candidate) - return nil, nil - } - if len(matchedResources) > 1 { - return nil, fmt.Errorf("multiple resources targeted by patch") - } - return matchedResources[0], nil -} - -func (m *merginator) mergeWithExisting( - index int, alreadyInResult *resource.Resource) error { - candidate := m.incoming[index] - cd, err := m.cdf.New(candidate.OrgId().Gvk) - if err != nil { - return err - } - hasConflict, err := cd.HasConflict(candidate, alreadyInResult) - if err != nil { - return err - } - if hasConflict { - return m.makeError(cd, index) - } - merged, err := cd.MergePatches(alreadyInResult, candidate) - if err != nil { - return err - } - _, err = m.result.Replace(merged) - return err -} - -// Make an error message describing the conflict. -func (m *merginator) makeError(cd resource.ConflictDetector, index int) error { - conflict, err := m.findConflict(cd, index) - if err != nil { - return err - } - if conflict == nil { - return fmt.Errorf("expected conflict for %s", m.incoming[index].OrgId()) - } - conflictMap, _ := conflict.Map() - incomingIndexMap, _ := m.incoming[index].Map() - return fmt.Errorf( - "conflict between %#v at index %d and %#v", - incomingIndexMap, - index, - conflictMap, - ) -} - -// findConflict looks for a conflict in a resource slice. -// It returns the first conflict between the resource at index -// and some other resource. Two resources can only conflict if -// they have the same original ResId. -func (m *merginator) findConflict( - cd resource.ConflictDetector, index int) (*resource.Resource, error) { - targetId := m.incoming[index].OrgId() - for i, p := range m.incoming { - if i == index || !targetId.Equals(p.OrgId()) { - continue - } - conflict, err := cd.HasConflict(p, m.incoming[index]) - if err != nil { - return nil, err - } - if conflict { - return p, nil - } - } - return nil, nil -} diff --git a/api/resmap/reswrangler_test.go b/api/resmap/reswrangler_test.go index 3c3963b422..111f3b1cf0 100644 --- a/api/resmap/reswrangler_test.go +++ b/api/resmap/reswrangler_test.go @@ -21,7 +21,7 @@ import ( var depProvider = provider.NewDefaultDepProvider() var rf = depProvider.GetResourceFactory() -var rmF = NewFactory(rf, depProvider.GetConflictDetectorFactory()) +var rmF = NewFactory(rf) func doAppend(t *testing.T, w ResMap, r *resource.Resource) { err := w.Append(r) diff --git a/api/resource/conflictdetector.go b/api/resource/conflictdetector.go deleted file mode 100644 index f079cdd5ab..0000000000 --- a/api/resource/conflictdetector.go +++ /dev/null @@ -1,20 +0,0 @@ -// Copyright 2019 The Kubernetes Authors. -// SPDX-License-Identifier: Apache-2.0 - -package resource - -import "sigs.k8s.io/kustomize/api/resid" - -// ConflictDetector detects conflicts between resources. -type ConflictDetector interface { - // HasConflict returns true if the given resources have a conflict. - HasConflict(patch1, patch2 *Resource) (bool, error) - // Merge two resources into one. - MergePatches(patch1, patch2 *Resource) (*Resource, error) -} - -// ConflictDetectorFactory makes instances of ConflictDetector that know -// how to handle the given Group, Version, Kind tuple. -type ConflictDetectorFactory interface { - New(gvk resid.Gvk) (ConflictDetector, error) -} diff --git a/api/testutils/kusttest/harnessenhanced.go b/api/testutils/kusttest/harnessenhanced.go index 177eb1ace4..ba4a613c50 100644 --- a/api/testutils/kusttest/harnessenhanced.go +++ b/api/testutils/kusttest/harnessenhanced.go @@ -47,8 +47,7 @@ func MakeEnhancedHarness(t *testing.T) *HarnessEnhanced { } p := provider.NewDefaultDepProvider() resourceFactory := p.GetResourceFactory() - resmapFactory := resmap.NewFactory( - resourceFactory, p.GetConflictDetectorFactory()) + resmapFactory := resmap.NewFactory(resourceFactory) result := &HarnessEnhanced{ Harness: MakeHarness(t), diff --git a/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go b/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go index 9c4af520bf..3aeb5fc4fd 100644 --- a/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go +++ b/cmd/pluginator/internal/krmfunction/funcwrappersrc/main.go @@ -15,8 +15,7 @@ import ( func main() { var plugin resmap.Configurable p := provider.NewDefaultDepProvider() - resmapFactory := resmap.NewFactory( - p.GetResourceFactory(), p.GetConflictDetectorFactory()) + resmapFactory := resmap.NewFactory(p.GetResourceFactory()) pluginHelpers := resmap.NewPluginHelpers( nil, p.GetFieldValidator(), resmapFactory) diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go index 6dfce8ae22..c566e2b35a 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go +++ b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go @@ -49,15 +49,6 @@ func (p *plugin) Config( return fmt.Errorf( "patch appears to be empty; files=%v, Patch=%s", p.Paths, p.Patches) } - // TODO(#3723): Delete conflict detection. - // Since #1500 closed, the conflict detector in use doesn't do - // anything useful. The resmap returned by this method hasn't - // been used for many releases. Leaving code as a comment to - // aid in deletion (fixing #3723). - // _, err = h.ResmapFactory().ConflatePatches(p.loadedPatches) - // if err != nil { - // return err - // } return nil } diff --git a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go index c75fc01672..a13ec13c35 100644 --- a/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go +++ b/plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer_test.go @@ -12,9 +12,6 @@ import ( kusttest_test "sigs.k8s.io/kustomize/api/testutils/kusttest" ) -// TODO(#3304): DECISION - OK to move to kyaml and not do conflict detection. -const skipConflictDetectionTests = true - func errorContains(err error, possibilities ...string) bool { for _, x := range possibilities { if strings.Contains(err.Error(), x) { @@ -317,60 +314,6 @@ spec: `) } -func TestStrategicMergeTransformerMultiplePatchesWithConflicts(t *testing.T) { - if skipConflictDetectionTests { - t.Skip("Skipping patch merge conflict tests.") - } - th := kusttest_test.MakeEnhancedHarness(t). - PrepBuiltin("PatchStrategicMergeTransformer") - defer th.Reset() - - th.WriteF("patch1.yaml", ` -apiVersion: apps/v1 -metadata: - name: myDeploy -kind: Deployment -spec: - template: - spec: - containers: - - name: nginx - image: nginx:latest - env: - - name: SOMEENV - value: BAR -`) - - th.WriteF("patch2.yaml", ` -apiVersion: apps/v1 -metadata: - name: myDeploy -kind: Deployment -spec: - template: - spec: - containers: - - name: nginx - image: nginx:1.7.9 - env: - - name: ANOTHERENV - value: HELLO - - name: busybox - image: busybox -`) - _, err := th.RunTransformer(` -apiVersion: builtin -kind: PatchStrategicMergeTransformer -metadata: - name: notImportantHere -paths: -- patch1.yaml -- patch2.yaml -`, target) - if assert.Error(t, err) && !errorContains(err, "conflict") { - t.Fatalf("expected error to contain %q but get %v", "conflict", err) - } -} func TestStrategicMergeTransformerWrongNamespace(t *testing.T) { th := kusttest_test.MakeEnhancedHarness(t). @@ -558,7 +501,7 @@ spec: `) } -func TestStrategicMergeTransformerNoSchemaMultiPatchesNoConflict(t *testing.T) { +func TestStrategicMergeTransformerNoSchemaMultiPatches(t *testing.T) { th := kusttest_test.MakeEnhancedHarness(t). PrepBuiltin("PatchStrategicMergeTransformer") defer th.Reset() @@ -652,47 +595,6 @@ spec: `) } -func TestStrategicMergeTransformerNoSchemaMultiPatchesWithConflict(t *testing.T) { - if skipConflictDetectionTests { - t.Skip("Skipping patch merge conflict tests.") - } - th := kusttest_test.MakeEnhancedHarness(t). - PrepBuiltin("PatchStrategicMergeTransformer") - defer th.Reset() - - th.WriteF("patch1.yaml", ` -apiVersion: example.com/v1 -kind: Foo -metadata: - name: my-foo -spec: - bar: - C: Z -`) - th.WriteF("patch2.yaml", ` -apiVersion: example.com/v1 -kind: Foo -metadata: - name: my-foo -spec: - bar: - C: NOT_Z - -`) - _, err := th.RunTransformer(` -apiVersion: builtin -kind: PatchStrategicMergeTransformer -metadata: - name: notImportantHere -paths: -- patch1.yaml -- patch2.yaml -`, targetNoschema) - if assert.Error(t, err) && !errorContains(err, "conflict") { - t.Fatalf("expected error to contain %q but get %v", "conflict", err) - } -} - // simple utility function to add an namespace in a resource // used as base, patch or expected result. Simply looks // for specs: in order to add namespace: xxxx before this line @@ -1049,9 +951,8 @@ func TestMultiplePatches(t *testing.T) { changeImagePatch(MyCRD, "nginx:latest"), }, errorExpected: false, - // There is no conflict detected. It should - // be but the JMPConflictDector ignores it. - // See https://github.com/kubernetes-sigs/kustomize/issues/1370 + // Theses patches aren't commutable (you get a different result + // if they are ordered differently). This is allowed without error. expected: expectedResultJMP("nginx:latest"), }, "noschema-latest-label-1.7.9": { @@ -1062,9 +963,8 @@ func TestMultiplePatches(t *testing.T) { changeImagePatch(MyCRD, "nginx:1.7.9"), }, errorExpected: false, - // There is no conflict detected. It should - // be but the JMPConflictDector ignores it. - // See https://github.com/kubernetes-sigs/kustomize/issues/1370 + // Theses patches aren't commutable (you get a different result + // if they are ordered differently). This is allowed without error. expected: expectedResultJMP("nginx:1.7.9"), }, } @@ -1090,127 +990,6 @@ func TestMultiplePatches(t *testing.T) { } } -// TestMultiplePatchesWithConflict checks that the conflict are -// detected regardless of the order of the patches and regardless -// of the schema availibility (SMP vs JSON) -func TestMultiplePatchesWithConflict(t *testing.T) { - if skipConflictDetectionTests { - t.Skip("Skipping patch merge conflict tests.") - } - tests := map[string]testRecord{ - "withschema-label-latest-1.7.9": { - base: baseResource(Deployment), - patch: []string{ - addLabelAndEnvPatch(Deployment), - changeImagePatch(Deployment, "nginx:latest"), - changeImagePatch(Deployment, "nginx:1.7.9"), - }, - errorExpected: true, - errorMsg: "conflict", - }, - "withschema-latest-label-1.7.9-difforder": { - base: baseResource(Deployment), - patch: []string{ - changeImagePatch(Deployment, "nginx:latest"), - addLabelAndEnvPatch(Deployment), - changeImagePatch(Deployment, "nginx:1.7.9"), - }, - errorExpected: true, - errorMsg: "conflict", - }, - "withschema-1.7.9-label-latest": { - base: baseResource(Deployment), - patch: []string{ - changeImagePatch(Deployment, "nginx:1.7.9"), - addLabelAndEnvPatch(Deployment), - changeImagePatch(Deployment, "nginx:latest"), - }, - errorExpected: true, - errorMsg: "conflict", - }, - "withschema-1.7.9-latest-label": { - base: baseResource(Deployment), - patch: []string{ - changeImagePatch(Deployment, "nginx:1.7.9"), - changeImagePatch(Deployment, "nginx:latest"), - addLabelAndEnvPatch(Deployment), - changeImagePatch(Deployment, "nginx:nginx"), - }, - errorExpected: true, - errorMsg: "conflict", - }, - "noschema-label-latest-1.7.9": { - base: baseResource(MyCRD), - patch: []string{ - addLabelAndEnvPatch(MyCRD), - changeImagePatch(MyCRD, "nginx:latest"), - changeImagePatch(MyCRD, "nginx:1.7.9"), - }, - errorExpected: true, - errorMsg: "conflict", - }, - "noschema-1.7.9-latest-label": { - base: baseResource(MyCRD), - patch: []string{ - changeImagePatch(MyCRD, "nginx:1.7.9"), - changeImagePatch(MyCRD, "nginx:latest"), - addLabelAndEnvPatch(MyCRD), - changeImagePatch(MyCRD, "nginx:nginx"), - }, - errorExpected: true, - }, - "noschema-label-image-container": { - base: baseResource(MyCRD), - patch: []string{ - addLabelAndEnvPatch(MyCRD), - changeImagePatch(MyCRD, "nginx:latest"), - addContainerAndEnvPatch(MyCRD), - }, - errorExpected: true, - errorMsg: "conflict", - }, - "noschema-image-container-label": { - base: baseResource(MyCRD), - patch: []string{ - changeImagePatch(MyCRD, "nginx:latest"), - addContainerAndEnvPatch(MyCRD), - addLabelAndEnvPatch(MyCRD), - }, - errorExpected: true, - errorMsg: "conflict", - }, - "noschema-container-label-image": { - base: baseResource(MyCRD), - patch: []string{ - addContainerAndEnvPatch(MyCRD), - addLabelAndEnvPatch(MyCRD), - changeImagePatch(MyCRD, "nginx:latest"), - }, - errorExpected: true, - errorMsg: "conflict", - }, - } - th := kusttest_test.MakeEnhancedHarness(t). - PrepBuiltin("PatchStrategicMergeTransformer") - defer th.Reset() - - for name, test := range tests { - t.Run(name, func(t *testing.T) { - th.ResetLoaderRoot(fmt.Sprintf("/%s", name)) - for idx, patch := range test.patch { - th.WriteF(fmt.Sprintf("/%s/patch%d.yaml", name, idx), patch) - } - if test.errorExpected { - _, err := th.RunTransformer(toConfig(test.patch...), test.base) - compareExpectedError(t, name, err, test.errorMsg) - } else { - th.RunTransformerAndCheckResult( - toConfig(test.patch...), test.base, test.expected) - } - }) - } -} - // TestMultipleNamespaces before the same patch // on two objects have the same name but in a different namespaces func TestMultipleNamespaces(t *testing.T) {