Merge pull request #2880 from monopole/merginator

Simplify use of the Merginator.

api/builtins/PatchStrategicMergeTransformer.go

@@ -64,7 +64,7 @@ func (p *PatchStrategicMergeTransformerPlugin) Config(
 }
 
 func (p *PatchStrategicMergeTransformerPlugin) Transform(m resmap.ResMap) error {
-	patches, err := p.h.ResmapFactory().MergePatches(p.loadedPatches)
+	patches, err := p.h.ResmapFactory().Merge(p.loadedPatches)
 	if err != nil {
 		return err
 	}

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.
+type merginatorImpl struct {
-// It errors out if there is conflict between patches.
+	rf *resource.Factory
-func MergePatches(patches []*resource.Resource,
+}
-	rf *resource.Factory) (resmap.ResMap, error) {
+
+// 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/k8sdeps/transformer/factory.go

@@ -1,25 +0,0 @@
-// Copyright 2019 The Kubernetes Authors.
-// SPDX-License-Identifier: Apache-2.0
-
-// Package transformer provides transformer factory
-package transformer
-
-import (
-	"sigs.k8s.io/kustomize/api/internal/k8sdeps/transformer/patch"
-	"sigs.k8s.io/kustomize/api/resmap"
-	"sigs.k8s.io/kustomize/api/resource"
-)
-
-// FactoryImpl makes patch transformer and name hash transformer
-type FactoryImpl struct{}
-
-// NewFactoryImpl makes a new factoryImpl instance
-func NewFactoryImpl() *FactoryImpl {
-	return &FactoryImpl{}
-}
-
-func (p *FactoryImpl) MergePatches(patches []*resource.Resource,
-	rf *resource.Factory) (
-	resmap.ResMap, error) {
-	return patch.MergePatches(patches, rf)
-}

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(
+	subKt := NewKustTarget(ldr, kt.validator, kt.rFactory, kt.pLdr)
-		ldr, kt.validator, kt.rFactory, kt.tFactory, 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 {
+	resourceFactory *resource.Factory) *target.KustTarget {
-	rf := resmap.NewFactory(resFact, transformer.NewFactoryImpl())
-	pc := konfig.DisabledPluginConfig()
 	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()
+	resourceFactory := resource.NewFactory(
-	rf := resmap.NewFactory(
+		kunstruct.NewKunstructuredFactoryImpl())
-		resource.NewFactory(
+	resmapFactory := resmap.NewFactory(
-			kunstruct.NewKunstructuredFactoryImpl()),
+		resourceFactory,
-		pf)
+		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,
+		resmapFactory,
-		pf,
+		pLdr.NewLoader(b.options.PluginConfig, resmapFactory),
-		pLdr.NewLoader(b.options.PluginConfig, rf),
 	)
 	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 {
+func NewFactory(rf *resource.Factory, pm Merginator) *Factory {
-	return &Factory{resF: rf, tf: tf}
+	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) (
+// Merge creates a new ResMap by merging incoming resources.
-	ResMap, error) {
+// Error if conflict found.
-	return rmF.tf.MergePatches(patches, rmF.resF)
+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)
+}

api/resmap/patchfactory.go

@@ -1,15 +0,0 @@
-/// Copyright 2019 The Kubernetes Authors.
-// SPDX-License-Identifier: Apache-2.0
-
-// Package patch holds miscellaneous interfaces used by kustomize.
-package resmap
-
-import (
-	"sigs.k8s.io/kustomize/api/resource"
-)
-
-// PatchFactory makes transformers that require k8sdeps.
-type PatchFactory interface {
-	MergePatches(patches []*resource.Resource,
-		rf *resource.Factory) (ResMap, error)
-}

api/testutils/kusttest/harnessenhanced.go

@@ -8,7 +8,7 @@ import (
 
 	"sigs.k8s.io/kustomize/api/filesys"
 	"sigs.k8s.io/kustomize/api/ifc"
-	"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/k8sdeps/kunstruct"
 	"sigs.k8s.io/kustomize/api/konfig"
@@ -46,16 +46,17 @@ func MakeEnhancedHarness(t *testing.T) *HarnessEnhanced {
 	if err != nil {
 		t.Fatal(err)
 	}
-
+	resourceFactory := resource.NewFactory(
-	rf := resmap.NewFactory(
+		kunstruct.NewKunstructuredFactoryImpl())
-		resource.NewFactory(kunstruct.NewKunstructuredFactoryImpl()),
+	resmapFactory := resmap.NewFactory(
-		transformer.NewFactoryImpl())
+		resourceFactory,
+		merge.NewMerginator(resourceFactory))
 
 	result := &HarnessEnhanced{
 		Harness: MakeHarness(t),
 		pte:     pte,
-		rf:      rf,
+		rf:      resmapFactory,
-		pl:      pLdr.NewLoader(pc, rf)}
+		pl:      pLdr.NewLoader(pc, resmapFactory)}
 
 	// Point the file loader to the root ('/') of the in-memory file system.
 	result.ResetLoaderRoot(filesys.Separator)

api/types/kustomization_test.go

@@ -5,9 +5,11 @@ import (
 )
 
 func fixKustomizationPostUnmarshallingCheck(k, e *Kustomization) bool {
-	return (k.Kind == e.Kind && k.APIVersion == e.APIVersion &&
+	return k.Kind == e.Kind &&
-		len(k.Resources) == len(e.Resources) && k.Resources[0] == e.Resources[0] &&
+		k.APIVersion == e.APIVersion &&
-		k.Bases == nil)
+		len(k.Resources) == len(e.Resources) &&
+		k.Resources[0] == e.Resources[0] &&
+		k.Bases == nil
 }
 
 func TestFixKustomizationPostUnmarshalling(t *testing.T) {

plugin/builtin/patchstrategicmergetransformer/PatchStrategicMergeTransformer.go

@@ -68,7 +68,7 @@ func (p *plugin) Config(
 }
 
 func (p *plugin) Transform(m resmap.ResMap) error {
-	patches, err := p.h.ResmapFactory().MergePatches(p.loadedPatches)
+	patches, err := p.h.ResmapFactory().Merge(p.loadedPatches)
 	if err != nil {
 		return err
 	}

releasing/releasing/gitrunner.go

@@ -63,14 +63,14 @@ func (gr *gitRunner) DeleteWorktreeDir() error {
 
 func (gr *gitRunner) WorktreePath() (string, error) {
 	if gr.worktreePath == "" {
-		return "", fmt.Errorf("Empty worktree path")
+		return "", fmt.Errorf("empty worktree path")
 	}
 	return gr.worktreePath, nil
 }
 
 func (gr *gitRunner) OriginalGitPath() (string, error) {
 	if gr.originalGitPath == "" {
-		return "", fmt.Errorf("Empty git path")
+		return "", fmt.Errorf("empty git path")
 	}
 	return gr.originalGitPath, nil
 }
@@ -107,7 +107,7 @@ func (gr *gitRunner) CheckRemoteExistence(remote string) error {
 	regString := fmt.Sprintf("(?m)^\\s*%s\\s*$", remote)
 	reg := regexp.MustCompile(regString)
 	if !reg.MatchString(string(stdoutStderr)) {
-		return fmt.Errorf("Cannot find remote named %s", remote)
+		return fmt.Errorf("cannot find remote named %s", remote)
 	}
 	logDebug("Remote %s exists", remote)
 	return nil

releasing/releasing/moduleversion.go

@@ -29,7 +29,7 @@ func (v *moduleVersion) Bump(t string) error {
 	} else if t == "patch" {
 		v.patch++
 	} else {
-		return fmt.Errorf("Invalid version type: %s", t)
+		return fmt.Errorf("invalid version type: %s", t)
 	}
 	return nil
 }
@@ -37,14 +37,14 @@ func (v *moduleVersion) Bump(t string) error {
 func newModuleVersionFromString(vs string) (moduleVersion, error) {
 	v := moduleVersion{}
 	if len(vs) < 1 {
-		return v, fmt.Errorf("Invalid version string %s", vs)
+		return v, fmt.Errorf("invalid version string %s", vs)
 	}
 	if vs[0] == 'v' {
 		vs = vs[1:]
 	}
 	versions := strings.Split(vs, ".")
 	if len(versions) != 3 {
-		return v, fmt.Errorf("Invalid version string %s", vs)
+		return v, fmt.Errorf("invalid version string %s", vs)
 	}
 	major, err := strconv.Atoi(versions[0])
 	if err != nil {