Merge pull request #2880 from monopole/merginator

Simplify use of the Merginator.

api/internal/k8sdeps/transformer/patch/conflictdetector.go → api/internal/k8sdeps/merge/merginator.go

@@ -1,7 +1,7 @@
 // Copyright 2019 The Kubernetes Authors.
 // SPDX-License-Identifier: Apache-2.0
 
-package patch
+package merge
 
 import (
 	"encoding/json"
@@ -20,18 +20,20 @@ import (
 
 type conflictDetector interface {
 	hasConflict(patch1, patch2 *resource.Resource) (bool, error)
-	findConflict(conflictingPatchIdx int, patches []*resource.Resource) (*resource.Resource, error)
+	findConflict(
+		conflictingPatchIdx int,
+		patches []*resource.Resource) (*resource.Resource, error)
 	mergePatches(patch1, patch2 *resource.Resource) (*resource.Resource, error)
 }
 
 type jsonMergePatch struct {
-	rf *resource.Factory
+	resourceFactory *resource.Factory
 }
 
 var _ conflictDetector = &jsonMergePatch{}
 
 func newJMPConflictDetector(rf *resource.Factory) conflictDetector {
-	return &jsonMergePatch{rf: rf}
+	return &jsonMergePatch{resourceFactory: rf}
 }
 
 func (jmp *jsonMergePatch) hasConflict(
@@ -40,7 +42,8 @@ func (jmp *jsonMergePatch) hasConflict(
 }
 
 func (jmp *jsonMergePatch) findConflict(
-	conflictingPatchIdx int, patches []*resource.Resource) (*resource.Resource, error) {
+	conflictingPatchIdx int,
+	patches []*resource.Resource) (*resource.Resource, error) {
 	for i, patch := range patches {
 		if i == conflictingPatchIdx {
 			continue
@@ -77,7 +80,7 @@ func (jmp *jsonMergePatch) mergePatches(
 	}
 	mergedMap := make(map[string]interface{})
 	err = json.Unmarshal(mergedBytes, &mergedMap)
-	return jmp.rf.FromMap(mergedMap), err
+	return jmp.resourceFactory.FromMap(mergedMap), err
 }
 
 type strategicMergePatch struct {
@@ -94,13 +97,15 @@ func newSMPConflictDetector(
 	return &strategicMergePatch{lookupPatchMeta: lookupPatchMeta, rf: rf}, err
 }
 
-func (smp *strategicMergePatch) hasConflict(p1, p2 *resource.Resource) (bool, error) {
+func (smp *strategicMergePatch) hasConflict(
+	p1, p2 *resource.Resource) (bool, error) {
 	return strategicpatch.MergingMapsHaveConflicts(
 		p1.Map(), p2.Map(), smp.lookupPatchMeta)
 }
 
 func (smp *strategicMergePatch) findConflict(
-	conflictingPatchIdx int, patches []*resource.Resource) (*resource.Resource, error) {
+	conflictingPatchIdx int,
+	patches []*resource.Resource) (*resource.Resource, error) {
 	for i, patch := range patches {
 		if i == conflictingPatchIdx {
 			continue
@@ -122,10 +127,12 @@ func (smp *strategicMergePatch) findConflict(
 	return nil, nil
 }
 
-func (smp *strategicMergePatch) mergePatches(patch1, patch2 *resource.Resource) (*resource.Resource, error) {
+func (smp *strategicMergePatch) mergePatches(
+	patch1, patch2 *resource.Resource) (*resource.Resource, error) {
 	if hasDeleteDirectiveMarker(patch2.Map()) {
 		if hasDeleteDirectiveMarker(patch1.Map()) {
-			return nil, fmt.Errorf("cannot merge patches both containing '$patch: delete' directives")
+			return nil, fmt.Errorf(
+				"cannot merge patches both containing '$patch: delete' directives")
 		}
 		patch1, patch2 = patch2, patch1
 	}
@@ -134,10 +141,21 @@ func (smp *strategicMergePatch) mergePatches(patch1, patch2 *resource.Resource)
 	return smp.rf.FromMap(mergeJSONMap), err
 }
 
-// MergePatches merge and index patches by OrgId.
-// It errors out if there is conflict between patches.
-func MergePatches(patches []*resource.Resource,
-	rf *resource.Factory) (resmap.ResMap, error) {
+type merginatorImpl struct {
+	rf *resource.Factory
+}
+
+// NewMerginator returns a new implementation of resmap.Merginator.
+func NewMerginator(rf *resource.Factory) resmap.Merginator {
+	return &merginatorImpl{rf: rf}
+}
+
+var _ resmap.Merginator = (*merginatorImpl)(nil)
+
+// Merge merges the incoming resources into a new resmap.ResMap.
+// Returns an error on conflict.
+func (m *merginatorImpl) Merge(
+	patches []*resource.Resource) (resmap.ResMap, error) {
 	rc := resmap.New()
 	for ix, patch := range patches {
 		id := patch.OrgId()
@@ -156,9 +174,9 @@ func MergePatches(patches []*resource.Resource,
 		}
 		var cd conflictDetector
 		if err != nil {
-			cd = newJMPConflictDetector(rf)
+			cd = newJMPConflictDetector(m.rf)
 		} else {
-			cd, err = newSMPConflictDetector(versionedObj, rf)
+			cd, err = newSMPConflictDetector(versionedObj, m.rf)
 			if err != nil {
 				return nil, err
 			}

api/internal/plugins/fnplugin/fnplugin.go

@@ -173,11 +173,11 @@ func (p *FnPlugin) invokePlugin(input []byte) ([]byte, error) {
 	// TODO(donnyxia): This is actually not used by generator and only used to bypass a kio limitation.
 	// Need better solution.
 	if input == nil {
-		yaml, err := functionConfig.String()
+		yml, err := functionConfig.String()
 		if err != nil {
 			return nil, err
 		}
-		input = []byte(yaml)
+		input = []byte(yml)
 	}
 
 	// Configure and Execute Fn. We don't need to convert resources to ResourceList here

api/internal/target/kusttarget.go

@@ -27,7 +27,6 @@ type KustTarget struct {
 	ldr           ifc.Loader
 	validator     ifc.Validator
 	rFactory      *resmap.Factory
-	tFactory      resmap.PatchFactory
 	pLdr          *loader.Loader
 }
 
@@ -36,13 +35,11 @@ func NewKustTarget(
 	ldr ifc.Loader,
 	validator ifc.Validator,
 	rFactory *resmap.Factory,
-	tFactory resmap.PatchFactory,
 	pLdr *loader.Loader) *KustTarget {
 	return &KustTarget{
 		ldr:       ldr,
 		validator: validator,
 		rFactory:  rFactory,
-		tFactory:  tFactory,
 		pLdr:      pLdr,
 	}
 }
@@ -350,8 +347,7 @@ func (kt *KustTarget) accumulateComponents(
 func (kt *KustTarget) accumulateDirectory(
 	ra *accumulator.ResAccumulator, ldr ifc.Loader, isComponent bool) (*accumulator.ResAccumulator, error) {
 	defer ldr.Cleanup()
-	subKt := NewKustTarget(
-		ldr, kt.validator, kt.rFactory, kt.tFactory, kt.pLdr)
+	subKt := NewKustTarget(ldr, kt.validator, kt.rFactory, kt.pLdr)
 	err := subKt.Load()
 	if err != nil {
 		return nil, errors.Wrapf(

api/internal/target/maker_test.go

@@ -7,7 +7,7 @@ import (
 	"testing"
 
 	"sigs.k8s.io/kustomize/api/filesys"
-	"sigs.k8s.io/kustomize/api/internal/k8sdeps/transformer"
+	"sigs.k8s.io/kustomize/api/internal/k8sdeps/merge"
 	pLdr "sigs.k8s.io/kustomize/api/internal/plugins/loader"
 	"sigs.k8s.io/kustomize/api/internal/target"
 	"sigs.k8s.io/kustomize/api/k8sdeps/kunstruct"
@@ -35,17 +35,17 @@ func makeKustTargetWithRf(
 	t *testing.T,
 	fSys filesys.FileSystem,
 	root string,
-	resFact *resource.Factory) *target.KustTarget {
-	rf := resmap.NewFactory(resFact, transformer.NewFactoryImpl())
-	pc := konfig.DisabledPluginConfig()
+	resourceFactory *resource.Factory) *target.KustTarget {
 	ldr, err := fLdr.NewLoader(fLdr.RestrictionRootOnly, root, fSys)
 	if err != nil {
 		t.Fatal(err)
 	}
+	rf := resmap.NewFactory(
+		resourceFactory, merge.NewMerginator(resourceFactory))
+	pc := konfig.DisabledPluginConfig()
 	return target.NewKustTarget(
 		ldr,
 		valtest_test.MakeFakeValidator(),
 		rf,
-		transformer.NewFactoryImpl(),
 		pLdr.NewLoader(pc, rf))
 }

api/krusty/kustomizer.go

@@ -8,7 +8,7 @@ import (
 
 	"sigs.k8s.io/kustomize/api/builtins"
 	"sigs.k8s.io/kustomize/api/filesys"
-	"sigs.k8s.io/kustomize/api/internal/k8sdeps/transformer"
+	"sigs.k8s.io/kustomize/api/internal/k8sdeps/merge"
 	pLdr "sigs.k8s.io/kustomize/api/internal/plugins/loader"
 	"sigs.k8s.io/kustomize/api/internal/target"
 	"sigs.k8s.io/kustomize/api/k8sdeps/kunstruct"
@@ -49,11 +49,11 @@ func MakeKustomizer(fSys filesys.FileSystem, o *Options) *Kustomizer {
 // on any number of internal paths (e.g. the filesystem may contain
 // multiple overlays, and Run can be called on each of them).
 func (b *Kustomizer) Run(path string) (resmap.ResMap, error) {
-	pf := transformer.NewFactoryImpl()
-	rf := resmap.NewFactory(
-		resource.NewFactory(
-			kunstruct.NewKunstructuredFactoryImpl()),
-		pf)
+	resourceFactory := resource.NewFactory(
+		kunstruct.NewKunstructuredFactoryImpl())
+	resmapFactory := resmap.NewFactory(
+		resourceFactory,
+		merge.NewMerginator(resourceFactory))
 	lr := fLdr.RestrictionNone
 	if b.options.LoadRestrictions == types.LoadRestrictionsRootOnly {
 		lr = fLdr.RestrictionRootOnly
@@ -66,9 +66,8 @@ func (b *Kustomizer) Run(path string) (resmap.ResMap, error) {
 	kt := target.NewKustTarget(
 		ldr,
 		validator.NewKustValidator(),
-		rf,
-		pf,
-		pLdr.NewLoader(b.options.PluginConfig, rf),
+		resmapFactory,
+		pLdr.NewLoader(b.options.PluginConfig, resmapFactory),
 	)
 	err = kt.Load()
 	if err != nil {
@@ -84,7 +83,9 @@ func (b *Kustomizer) Run(path string) (resmap.ResMap, error) {
 	}
 	if b.options.AddManagedbyLabel {
 		t := builtins.LabelTransformerPlugin{
-			Labels: map[string]string{konfig.ManagedbyLabelKey: fmt.Sprintf("kustomize-%s", provenance.GetProvenance().Version)},
+			Labels: map[string]string{
+				konfig.ManagedbyLabelKey: fmt.Sprintf(
+					"kustomize-%s", provenance.GetProvenance().Version)},
 			FieldSpecs: []types.FieldSpec{{
 				Path:               "metadata/labels",
 				CreateIfNotPresent: true,

api/loader/fileloader.go

@@ -199,7 +199,9 @@ func (fl *fileLoader) New(path string) (ifc.Loader, error) {
 	}
 	root, errDir := demandDirectoryRoot(fl.fSys, fl.root.Join(path))
 	if errDir != nil {
-		return nil, fmt.Errorf("Error loading %s with git: %v, dir: %v, get: %v", path, errGit, errDir, errGet)
+		return nil, fmt.Errorf(
+			"error loading %s with git: %v, dir: %v, get: %v",
+			path, errGit, errDir, errGet)
 	}
 	if errDir := fl.errIfGitContainmentViolation(root); errDir != nil {
 		return nil, errDir

api/loader/loader.go

@@ -39,5 +39,7 @@ func NewLoader(
 		return newLoaderAtConfirmedDir(lr, root, fSys, nil, git.ClonerUsingGitExec, getRemoteTarget), nil
 	}
 
-	return nil, fmt.Errorf("Error creating new loader with git: %v, dir: %v, get: %v", errGit, errDir, errGet)
+	return nil, fmt.Errorf(
+		"error creating new loader with git: %v, dir: %v, get: %v",
+		errGit, errDir, errGet)
 }

api/resmap/factory.go

@@ -14,12 +14,12 @@ import (
 // Factory makes instances of ResMap.
 type Factory struct {
 	resF *resource.Factory
-	tf   PatchFactory
+	pm   Merginator
 }
 
 // NewFactory returns a new resmap.Factory.
-func NewFactory(rf *resource.Factory, tf PatchFactory) *Factory {
-	return &Factory{resF: rf, tf: tf}
+func NewFactory(rf *resource.Factory, pm Merginator) *Factory {
+	return &Factory{resF: rf, pm: pm}
 }
 
 // RF returns a resource.Factory.
@@ -87,6 +87,7 @@ func (rmF *Factory) NewResMapFromConfigMapArgs(
 	return newResMapFromResourceSlice(resources)
 }
 
+// FromConfigMapArgs creates a new ResMap containing one ConfigMap.
 func (rmF *Factory) FromConfigMapArgs(
 	kvLdr ifc.KvLoader, args types.ConfigMapArgs) (ResMap, error) {
 	res, err := rmF.resF.MakeConfigMap(kvLdr, &args)
@@ -111,6 +112,7 @@ func (rmF *Factory) NewResMapFromSecretArgs(
 	return newResMapFromResourceSlice(resources)
 }
 
+// FromSecretArgs creates a new ResMap containing one secret.
 func (rmF *Factory) FromSecretArgs(
 	kvLdr ifc.KvLoader, args types.SecretArgs) (ResMap, error) {
 	res, err := rmF.resF.MakeSecret(kvLdr, &args)
@@ -120,12 +122,14 @@ func (rmF *Factory) FromSecretArgs(
 	return rmF.FromResource(res), nil
 }
 
-func (rmF *Factory) MergePatches(patches []*resource.Resource) (
-	ResMap, error) {
-	return rmF.tf.MergePatches(patches, rmF.resF)
+// Merge creates a new ResMap by merging incoming resources.
+// Error if conflict found.
+func (rmF *Factory) Merge(patches []*resource.Resource) (ResMap, error) {
+	return rmF.pm.Merge(patches)
 }
 
-func newResMapFromResourceSlice(resources []*resource.Resource) (ResMap, error) {
+func newResMapFromResourceSlice(
+	resources []*resource.Resource) (ResMap, error) {
 	result := New()
 	for _, res := range resources {
 		err := result.Append(res)

api/resmap/merginator.go

@@ -0,0 +1,13 @@
+// Copyright 2019 The Kubernetes Authors.
+// SPDX-License-Identifier: Apache-2.0
+
+package resmap
+
+import "sigs.k8s.io/kustomize/api/resource"
+
+// Merginator merges resources.
+type Merginator interface {
+	// Merge creates a new ResMap by merging incoming resources.
+	// Error if conflict found.
+	Merge([]*resource.Resource) (ResMap, error)
+}