Merge pull request #195 from monopole/tightenUp

Pull factories up call stack (make them less often).

pkg/app/application.go

@@ -26,6 +26,7 @@ import (
 	"github.com/ghodss/yaml"
 	"github.com/golang/glog"
 
+	"github.com/kubernetes-sigs/kustomize/pkg/configmapandsecret"
 	"github.com/kubernetes-sigs/kustomize/pkg/constants"
 	"github.com/kubernetes-sigs/kustomize/pkg/crds"
 	"github.com/kubernetes-sigs/kustomize/pkg/fs"
@@ -44,13 +45,13 @@ import (
 // https://github.com/kubernetes-sigs/kustomize/blob/master/docs/glossary.md#target
 type Application struct {
 	kustomization *types.Kustomization
-	loader        loader.Loader
+	ldr           loader.Loader
 	fSys          fs.FileSystem
 }
 
 // NewApplication returns a new instance of Application primed with a Loader.
-func NewApplication(loader loader.Loader, fSys fs.FileSystem) (*Application, error) {
+func NewApplication(ldr loader.Loader, fSys fs.FileSystem) (*Application, error) {
-	content, err := loader.Load(constants.KustomizationFileName)
+	content, err := ldr.Load(constants.KustomizationFileName)
 	if err != nil {
 		return nil, err
 	}
@@ -60,7 +61,8 @@ func NewApplication(loader loader.Loader, fSys fs.FileSystem) (*Application, err
 	if err != nil {
 		return nil, err
 	}
-	return &Application{kustomization: &m, loader: loader, fSys: fSys}, nil
+
+	return &Application{kustomization: &m, ldr: ldr, fSys: fSys}, nil
 }
 
 func unmarshal(y []byte, o interface{}) error {
@@ -136,18 +138,19 @@ func (a *Application) loadCustomizedResMap() (resmap.ResMap, error) {
 	if err != nil {
 		errs.Append(errors.Wrap(err, "loadResMapFromBasesAndResources"))
 	}
-	err = crds.RegisterCRDs(a.loader, a.kustomization.CRDs)
+	err = crds.RegisterCRDs(a.ldr, a.kustomization.CRDs)
 	if err != nil {
 		errs.Append(errors.Wrap(err, "RegisterCRDs"))
 	}
-
 	cms, err := resmap.NewResMapFromConfigMapArgs(
-		a.loader, a.fSys, a.kustomization.ConfigMapGenerator)
+		configmapandsecret.NewConfigMapFactory(a.fSys, a.ldr),
+		a.kustomization.ConfigMapGenerator)
 	if err != nil {
 		errs.Append(errors.Wrap(err, "NewResMapFromConfigMapArgs"))
 	}
 	secrets, err := resmap.NewResMapFromSecretArgs(
-		a.loader.Root(), a.fSys, a.kustomization.SecretGenerator)
+		configmapandsecret.NewSecretFactory(a.fSys, a.ldr.Root()),
+		a.kustomization.SecretGenerator)
 	if err != nil {
 		errs.Append(errors.Wrap(err, "NewResMapFromSecretArgs"))
 	}
@@ -161,7 +164,7 @@ func (a *Application) loadCustomizedResMap() (resmap.ResMap, error) {
 		return nil, err
 	}
 
-	patches, err := resmap.NewResourceSliceFromPatches(a.loader, a.kustomization.Patches)
+	patches, err := resmap.NewResourceSliceFromPatches(a.ldr, a.kustomization.Patches)
 	if err != nil {
 		errs.Append(errors.Wrap(err, "NewResourceSliceFromPatches"))
 	}
@@ -192,7 +195,7 @@ func (a *Application) loadCustomizedResMap() (resmap.ResMap, error) {
 // Gets Bases and Resources as advertised.
 func (a *Application) loadResMapFromBasesAndResources() (resmap.ResMap, error) {
 	bases, errs := a.loadCustomizedBases()
-	resources, err := resmap.NewResMapFromFiles(a.loader, a.kustomization.Resources)
+	resources, err := resmap.NewResMapFromFiles(a.ldr, a.kustomization.Resources)
 	if err != nil {
 		errs.Append(errors.Wrap(err, "rawResources failed to read Resources"))
 	}
@@ -208,7 +211,7 @@ func (a *Application) loadCustomizedBases() (resmap.ResMap, *interror.Kustomizat
 	var list []resmap.ResMap
 	errs := &interror.KustomizationErrors{}
 	for _, path := range a.kustomization.Bases {
-		ldr, err := a.loader.New(path)
+		ldr, err := a.ldr.New(path)
 		if err != nil {
 			errs.Append(errors.Wrap(err, "couldn't make ldr for "+path))
 			continue
@@ -236,7 +239,7 @@ func (a *Application) loadBasesAsFlatList() ([]*Application, error) {
 	var result []*Application
 	errs := &interror.KustomizationErrors{}
 	for _, path := range a.kustomization.Bases {
-		ldr, err := a.loader.New(path)
+		ldr, err := a.ldr.New(path)
 		if err != nil {
 			errs.Append(err)
 			continue

pkg/commands/configmap.go

@@ -59,9 +59,10 @@ func newCmdAddConfigMap(fSys fs.FileSystem) *cobra.Command {
 			if err != nil {
 				return err
 			}
-
 			// Add the flagsAndArgs map to the kustomization file.
-			err = addConfigMap(kustomization, flagsAndArgs, fSys)
+			err = addConfigMap(
+				kustomization, flagsAndArgs,
+				configmapandsecret.NewConfigMapFactory(fSys, nil))
 			if err != nil {
 				return err
 			}
@@ -93,24 +94,22 @@ func newCmdAddConfigMap(fSys fs.FileSystem) *cobra.Command {
 }
 
 // addConfigMap adds a configmap to a kustomization file.
-// Note: error may leave kustomization file in an undefined state. Suggest passing a copy
+// Note: error may leave kustomization file in an undefined state.
-// of kustomization file.
+// Suggest passing a copy of kustomization file.
-func addConfigMap(k *types.Kustomization, flagsAndArgs cMapFlagsAndArgs, fSys fs.FileSystem) error {
+func addConfigMap(
+	k *types.Kustomization,
+	flagsAndArgs cMapFlagsAndArgs,
+	factory *configmapandsecret.ConfigMapFactory) error {
 	cmArgs := makeConfigMapArgs(k, flagsAndArgs.Name)
-
 	err := mergeFlagsIntoCmArgs(&cmArgs.DataSources, flagsAndArgs)
 	if err != nil {
 		return err
 	}
-
-	factory := configmapandsecret.NewConfigMapFactory(cmArgs, nil, fSys)
-
 	// Validate by trying to create corev1.configmap.
-	_, _, err = factory.MakeUnstructAndGenerateName()
+	_, _, err = factory.MakeUnstructAndGenerateName(cmArgs)
 	if err != nil {
 		return err
 	}
-
 	return nil
 }
 

pkg/configmapandsecret/configmapfactory.go

@@ -38,22 +38,20 @@ import (
 
 // ConfigMapFactory makes ConfigMaps.
 type ConfigMapFactory struct {
-	args *types.ConfigMapArgs
 	fSys fs.FileSystem
 	ldr  loader.Loader
 }
 
 // NewConfigMapFactory returns a new ConfigMapFactory.
 func NewConfigMapFactory(
-	args *types.ConfigMapArgs,
+	fSys fs.FileSystem, l loader.Loader) *ConfigMapFactory {
-	l loader.Loader,
+	return &ConfigMapFactory{fSys: fSys, ldr: l}
-	fSys fs.FileSystem) *ConfigMapFactory {
-	return &ConfigMapFactory{args: args, ldr: l, fSys: fSys}
 }
 
 // MakeUnstructAndGenerateName returns an configmap and the name appended with a hash.
-func (f *ConfigMapFactory) MakeUnstructAndGenerateName() (*unstructured.Unstructured, string, error) {
+func (f *ConfigMapFactory) MakeUnstructAndGenerateName(
-	cm, err := f.MakeConfigMap1()
+	args *types.ConfigMapArgs) (*unstructured.Unstructured, string, error) {
+	cm, err := f.MakeConfigMap1(args)
 	if err != nil {
 		return nil, "", err
 	}
@@ -76,31 +74,32 @@ func objectToUnstructured(in runtime.Object) (*unstructured.Unstructured, error)
 	return &out, err
 }
 
-func (f *ConfigMapFactory) makeFreshConfigMap() *corev1.ConfigMap {
+func (f *ConfigMapFactory) makeFreshConfigMap(
+	args *types.ConfigMapArgs) *corev1.ConfigMap {
 	cm := &corev1.ConfigMap{}
 	cm.APIVersion = "v1"
 	cm.Kind = "ConfigMap"
-	cm.Name = f.args.Name
+	cm.Name = args.Name
 	cm.Data = map[string]string{}
 	return cm
 }
 
 // MakeConfigMap1 returns a new ConfigMap, or nil and an error.
-func (f *ConfigMapFactory) MakeConfigMap1() (*corev1.ConfigMap, error) {
+func (f *ConfigMapFactory) MakeConfigMap1(
-	cm := f.makeFreshConfigMap()
+	args *types.ConfigMapArgs) (*corev1.ConfigMap, error) {
-
+	cm := f.makeFreshConfigMap(args)
-	if f.args.EnvSource != "" {
+	if args.EnvSource != "" {
-		if err := f.handleConfigMapFromEnvFileSource(cm); err != nil {
+		if err := f.handleConfigMapFromEnvFileSource(cm, args); err != nil {
 			return nil, err
 		}
 	}
-	if f.args.FileSources != nil {
+	if args.FileSources != nil {
-		if err := f.handleConfigMapFromFileSources(cm); err != nil {
+		if err := f.handleConfigMapFromFileSources(cm, args); err != nil {
 			return nil, err
 		}
 	}
-	if f.args.LiteralSources != nil {
+	if args.LiteralSources != nil {
-		if err := f.handleConfigMapFromLiteralSources(cm); err != nil {
+		if err := f.handleConfigMapFromLiteralSources(cm, args); err != nil {
 			return nil, err
 		}
 	}
@@ -109,35 +108,30 @@ func (f *ConfigMapFactory) MakeConfigMap1() (*corev1.ConfigMap, error) {
 
 // MakeConfigMap2 returns a new ConfigMap, or nil and an error.
 // TODO: Get rid of the nearly duplicated code in MakeConfigMap1 vs MakeConfigMap2
-func (f *ConfigMapFactory) MakeConfigMap2() (*corev1.ConfigMap, error) {
+func (f *ConfigMapFactory) MakeConfigMap2(
+	args *types.ConfigMapArgs) (*corev1.ConfigMap, error) {
 	var envPairs, literalPairs, filePairs []kvPair
 	var err error
-
+	cm := f.makeFreshConfigMap(args)
-	cm := f.makeFreshConfigMap()
+	if args.EnvSource != "" {
-
+		envPairs, err = keyValuesFromEnvFile(f.ldr, args.EnvSource)
-	if f.args.EnvSource != "" {
-		envPairs, err = keyValuesFromEnvFile(f.ldr, f.args.EnvSource)
 		if err != nil {
 			return nil, fmt.Errorf(
 				"error reading keys from env source file: %s %v",
-				f.args.EnvSource, err)
+				args.EnvSource, err)
 		}
 	}
-
+	literalPairs, err = keyValuesFromLiteralSources(args.LiteralSources)
-	literalPairs, err = keyValuesFromLiteralSources(f.args.LiteralSources)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"error reading key values from literal sources: %v", err)
 	}
-
+	filePairs, err = keyValuesFromFileSources(f.ldr, args.FileSources)
-	filePairs, err = keyValuesFromFileSources(f.ldr, f.args.FileSources)
 	if err != nil {
 		return nil, fmt.Errorf(
 			"error reading key values from file sources: %v", err)
 	}
-
 	allPairs := append(append(envPairs, literalPairs...), filePairs...)
-
 	// merge key value pairs from all the sources
 	for _, kv := range allPairs {
 		err = addKV(cm.Data, kv)
@@ -163,8 +157,8 @@ func keyValuesFromLiteralSources(sources []string) ([]kvPair, error) {
 // handleConfigMapFromLiteralSources adds the specified literal source
 // information into the provided configMap.
 func (f *ConfigMapFactory) handleConfigMapFromLiteralSources(
-	configMap *v1.ConfigMap) error {
+	configMap *v1.ConfigMap, args *types.ConfigMapArgs) error {
-	for _, literalSource := range f.args.LiteralSources {
+	for _, literalSource := range args.LiteralSources {
 		keyName, value, err := ParseLiteralSource(literalSource)
 		if err != nil {
 			return err
@@ -195,8 +189,9 @@ func keyValuesFromFileSources(ldr loader.Loader, sources []string) ([]kvPair, er
 
 // handleConfigMapFromFileSources adds the specified file source information
 // into the provided configMap
-func (f *ConfigMapFactory) handleConfigMapFromFileSources(configMap *v1.ConfigMap) error {
+func (f *ConfigMapFactory) handleConfigMapFromFileSources(
-	for _, fileSource := range f.args.FileSources {
+	configMap *v1.ConfigMap, args *types.ConfigMapArgs) error {
+	for _, fileSource := range args.FileSources {
 		keyName, filePath, err := ParseFileSource(fileSource)
 		if err != nil {
 			return err
@@ -241,14 +236,15 @@ func keyValuesFromEnvFile(l loader.Loader, path string) ([]kvPair, error) {
 
 // HandleConfigMapFromEnvFileSource adds the specified env file source information
 // into the provided configMap
-func (f *ConfigMapFactory) handleConfigMapFromEnvFileSource(configMap *v1.ConfigMap) error {
+func (f *ConfigMapFactory) handleConfigMapFromEnvFileSource(
-	if !f.fSys.Exists(f.args.EnvSource) {
+	configMap *v1.ConfigMap, args *types.ConfigMapArgs) error {
-		return fmt.Errorf("unable to read configmap env file %s", f.args.EnvSource)
+	if !f.fSys.Exists(args.EnvSource) {
+		return fmt.Errorf("unable to read configmap env file %s", args.EnvSource)
 	}
-	if f.fSys.IsDir(f.args.EnvSource) {
+	if f.fSys.IsDir(args.EnvSource) {
-		return fmt.Errorf("env config file %s cannot be a directory", f.args.EnvSource)
+		return fmt.Errorf("env config file %s cannot be a directory", args.EnvSource)
 	}
-	return addFromEnvFile(f.args.EnvSource, func(key, value string) error {
+	return addFromEnvFile(args.EnvSource, func(key, value string) error {
 		return addKeyFromLiteralToConfigMap(configMap, key, value)
 	})
 }

pkg/configmapandsecret/configmapfactory_test.go

@@ -133,11 +133,10 @@ func TestConstructConfigMap(t *testing.T) {
 		},
 	}
 
-	for _, tc := range testCases {
 	// TODO: all tests should use a FakeFs
-		fSys := fs.MakeRealFS()
+	f := NewConfigMapFactory(fs.MakeRealFS(), nil)
-		f := NewConfigMapFactory(&tc.input, nil, fSys)
+	for _, tc := range testCases {
-		cm, err := f.MakeConfigMap1()
+		cm, err := f.MakeConfigMap1(&tc.input)
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
 		}

pkg/configmapandsecret/secretfactory.go

@@ -2,39 +2,40 @@ package configmapandsecret
 
 import (
 	"context"
+	"os/exec"
+	"path/filepath"
+	"time"
+
 	"github.com/kubernetes-sigs/kustomize/pkg/fs"
 	"github.com/kubernetes-sigs/kustomize/pkg/types"
 	"github.com/pkg/errors"
 	corev1 "k8s.io/api/core/v1"
-	"os/exec"
-	"path/filepath"
-	"time"
 )
 
 // SecretFactory makes Secrets.
 type SecretFactory struct {
-	args types.SecretArgs
 	fSys fs.FileSystem
+	wd   string
 }
 
 // NewSecretFactory returns a new SecretFactory.
-func NewSecretFactory(args types.SecretArgs, fSys fs.FileSystem) *SecretFactory {
+func NewSecretFactory(fSys fs.FileSystem, wd string) *SecretFactory {
-	return &SecretFactory{args: args, fSys: fSys}
+	return &SecretFactory{fSys: fSys, wd: wd}
 }
 
 // MakeSecret returns a new secret.
-func (f *SecretFactory) MakeSecret(wd string) (*corev1.Secret, error) {
+func (f *SecretFactory) MakeSecret(args types.SecretArgs) (*corev1.Secret, error) {
 	s := &corev1.Secret{}
 	s.APIVersion = "v1"
 	s.Kind = "Secret"
-	s.Name = f.args.Name
+	s.Name = args.Name
-	s.Type = corev1.SecretType(f.args.Type)
+	s.Type = corev1.SecretType(args.Type)
 	if s.Type == "" {
 		s.Type = corev1.SecretTypeOpaque
 	}
 	s.Data = map[string][]byte{}
-	for k, v := range f.args.Commands {
+	for k, v := range args.Commands {
-		out, err := f.createSecretKey(wd, v)
+		out, err := f.createSecretKey(v)
 		if err != nil {
 			return nil, errors.Wrap(err, "createSecretKey")
 		}
@@ -44,16 +45,16 @@ func (f *SecretFactory) MakeSecret(wd string) (*corev1.Secret, error) {
 }
 
 // Run a command, return its output as the secret.
-func (f *SecretFactory) createSecretKey(wd string, command string) ([]byte, error) {
+func (f *SecretFactory) createSecretKey(command string) ([]byte, error) {
-	if !f.fSys.IsDir(wd) {
+	if !f.fSys.IsDir(f.wd) {
-		wd = filepath.Dir(wd)
+		f.wd = filepath.Dir(f.wd)
-		if !f.fSys.IsDir(wd) {
+		if !f.fSys.IsDir(f.wd) {
-			return nil, errors.New("not a directory: " + wd)
+			return nil, errors.New("not a directory: " + f.wd)
 		}
 	}
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 	cmd := exec.CommandContext(ctx, "sh", "-c", command)
-	cmd.Dir = wd
+	cmd.Dir = f.wd
 	return cmd.Output()
 }

pkg/resmap/configmap.go

@@ -18,8 +18,6 @@ package resmap
 
 import (
 	"github.com/kubernetes-sigs/kustomize/pkg/configmapandsecret"
-	"github.com/kubernetes-sigs/kustomize/pkg/fs"
-	"github.com/kubernetes-sigs/kustomize/pkg/loader"
 	"github.com/kubernetes-sigs/kustomize/pkg/resource"
 	"github.com/kubernetes-sigs/kustomize/pkg/types"
 )
@@ -27,16 +25,14 @@ import (
 // NewResMapFromConfigMapArgs returns a Resource slice given
 // a configmap metadata slice from kustomization file.
 func NewResMapFromConfigMapArgs(
-	ldr loader.Loader,
+	f *configmapandsecret.ConfigMapFactory,
-	fSys fs.FileSystem,
 	cmArgsList []types.ConfigMapArgs) (ResMap, error) {
 	var allResources []*resource.Resource
 	for _, cmArgs := range cmArgsList {
 		if cmArgs.Behavior == "" {
 			cmArgs.Behavior = "create"
 		}
-		f := configmapandsecret.NewConfigMapFactory(&cmArgs, ldr, fSys)
+		cm, err := f.MakeConfigMap2(&cmArgs)
-		cm, err := f.MakeConfigMap2()
 		if err != nil {
 			return nil, err
 		}

pkg/resmap/configmap_test.go

@@ -20,6 +20,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/kubernetes-sigs/kustomize/pkg/configmapandsecret"
 	"github.com/kubernetes-sigs/kustomize/pkg/fs"
 	"github.com/kubernetes-sigs/kustomize/pkg/internal/loadertest"
 	"github.com/kubernetes-sigs/kustomize/pkg/resource"
@@ -39,6 +40,7 @@ func TestNewFromConfigMaps(t *testing.T) {
 	}
 
 	l := loadertest.NewFakeLoader("/home/seans/project/")
+	f := configmapandsecret.NewConfigMapFactory(fs.MakeFakeFS(), l)
 	testCases := []testCase{
 		{
 			description: "construct config map from env",
@@ -127,11 +129,10 @@ BAR=baz
 	}
 
 	for _, tc := range testCases {
-
 		if ferr := l.AddFile(tc.filepath, []byte(tc.content)); ferr != nil {
 			t.Fatalf("Error adding fake file: %v\n", ferr)
 		}
-		r, err := NewResMapFromConfigMapArgs(l, fs.MakeFakeFS(), tc.input)
+		r, err := NewResMapFromConfigMapArgs(f, tc.input)
 		if err != nil {
 			t.Fatalf("unexpected error: %v", err)
 		}

pkg/resmap/secret.go

@@ -18,20 +18,19 @@ package resmap
 
 import (
 	"github.com/kubernetes-sigs/kustomize/pkg/configmapandsecret"
-	"github.com/kubernetes-sigs/kustomize/pkg/fs"
 	"github.com/kubernetes-sigs/kustomize/pkg/resource"
 	"github.com/kubernetes-sigs/kustomize/pkg/types"
 	"github.com/pkg/errors"
 )
 
-// NewResMapFromSecretArgs takes a SecretArgs slice and executes its command in directory
+// NewResMapFromSecretArgs takes a SecretArgs slice, generates
-// wd then writes the output to a Resource slice and return it.
+// secrets from each entry, and accumulates them in a ResMap.
 func NewResMapFromSecretArgs(
-	wd string, fSys fs.FileSystem,
+	f *configmapandsecret.SecretFactory,
 	secretList []types.SecretArgs) (ResMap, error) {
 	var allResources []*resource.Resource
 	for _, args := range secretList {
-		s, err := configmapandsecret.NewSecretFactory(args, fSys).MakeSecret(wd)
+		s, err := f.MakeSecret(args)
 		if err != nil {
 			return nil, errors.Wrap(err, "makeSecret")
 		}

pkg/resmap/secret_test.go

@@ -21,6 +21,7 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/kubernetes-sigs/kustomize/pkg/configmapandsecret"
 	"github.com/kubernetes-sigs/kustomize/pkg/fs"
 	"github.com/kubernetes-sigs/kustomize/pkg/resource"
 	"github.com/kubernetes-sigs/kustomize/pkg/types"
@@ -43,7 +44,8 @@ func TestNewResMapFromSecretArgs(t *testing.T) {
 	}
 	fakeFs := fs.MakeFakeFS()
 	fakeFs.Mkdir(".")
-	actual, err := NewResMapFromSecretArgs(".", fakeFs, secrets)
+	actual, err := NewResMapFromSecretArgs(
+		configmapandsecret.NewSecretFactory(fakeFs, "."), secrets)
 
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)