Merge pull request #432 from bryanl/bug-param-list

bug: param list with env

actions/actions_test.go

@@ -80,6 +80,7 @@ func withApp(t *testing.T, fn func(*mocks.App)) {
 }
 
 func assertOutput(t *testing.T, filename, actual string) {
+	require.NotEmpty(t, filename, "filename can not be empty")
 	path := filepath.Join("testdata", filename)
 	b, err := ioutil.ReadFile(path)
 	require.NoError(t, err)

actions/param_list.go

@@ -25,6 +25,10 @@ import (
 	"github.com/pkg/errors"
 )
 
+type findModulesFn func(a app.App, envName string) ([]component.Module, error)
+type findModuleFn func(a app.App, moduleName string) (component.Module, error)
+type findComponentFn func(a app.App, moduleName, componentName string) (component.Component, error)
+
 // RunParamList runs `param list`.
 func RunParamList(m map[string]interface{}) error {
 	pl, err := NewParamList(m)
@@ -38,11 +42,14 @@ func RunParamList(m map[string]interface{}) error {
 // ParamList lists parameters for a component.
 type ParamList struct {
 	app           app.App
-	module        string
+	moduleName    string
 	componentName string
 	envName       string
-	cm            component.Manager
-	out           io.Writer
+
+	out             io.Writer
+	findModulesFn   findModulesFn
+	findModuleFn    findModuleFn
+	findComponentFn findComponentFn
 }
 
 // NewParamList creates an instances of ParamList.
@@ -51,12 +58,14 @@ func NewParamList(m map[string]interface{}) (*ParamList, error) {
 
 	pl := &ParamList{
 		app:           ol.loadApp(),
-		module:        ol.loadString(OptionModule),
-		componentName: ol.loadString(OptionComponentName),
-		envName:       ol.loadString(OptionEnvName),
-
-		cm:  component.DefaultManager,
-		out: os.Stdout,
+		moduleName:    ol.loadOptionalString(OptionModule),
+		componentName: ol.loadOptionalString(OptionComponentName),
+		envName:       ol.loadOptionalString(OptionEnvName),
+
+		out:             os.Stdout,
+		findModulesFn:   component.ModulesFromEnv,
+		findModuleFn:    component.GetModule,
+		findComponentFn: component.LocateComponent,
 	}
 
 	if ol.err != nil {
@@ -68,9 +77,13 @@ func NewParamList(m map[string]interface{}) (*ParamList, error) {
 
 // Run runs the ParamList action.
 func (pl *ParamList) Run() error {
-	module, err := pl.cm.Module(pl.app, pl.module)
+	if pl.envName != "" {
+		return pl.handleEnvParams()
+	}
+
+	module, err := pl.findModuleFn(pl.app, pl.moduleName)
 	if err != nil {
-		return errors.Wrap(err, "could not find namespace")
+		return errors.Wrap(err, "could not find module")
 	}
 
 	params, err := pl.collectParams(module)
@@ -78,16 +91,31 @@ func (pl *ParamList) Run() error {
 		return err
 	}
 
-	table := table.New(pl.out)
+	return pl.print(params)
+}
 
-	table.SetHeader([]string{"COMPONENT", "INDEX", "PARAM", "VALUE"})
-	for _, data := range params {
-		table.Append([]string{data.Component, data.Index, data.Key, data.Value})
+func (pl *ParamList) handleEnvParams() error {
+	modules, err := pl.findModulesFn(pl.app, pl.envName)
+	if err != nil {
+		return err
 	}
 
-	table.Render()
+	var params []component.ModuleParameter
+
+	for _, module := range modules {
+		moduleParams, err := pl.collectParams(module)
+		if err != nil {
+			return err
+		}
+
+		if pl.moduleName != "" && module.Name() != pl.moduleName {
+			continue
+		}
+		params = append(params, moduleParams...)
+	}
+
+	return pl.print(params)
 
-	return nil
 }
 
 func (pl *ParamList) collectParams(module component.Module) ([]component.ModuleParameter, error) {
@@ -95,10 +123,21 @@ func (pl *ParamList) collectParams(module component.Module) ([]component.ModuleP
 		return module.Params(pl.envName)
 	}
 
-	c, err := pl.cm.Component(pl.app, pl.module, pl.componentName)
+	c, err := pl.findComponentFn(pl.app, pl.moduleName, pl.componentName)
 	if err != nil {
 		return nil, err
 	}
 
 	return c.Params(pl.envName)
 }
+
+func (pl *ParamList) print(params []component.ModuleParameter) error {
+	table := table.New(pl.out)
+
+	table.SetHeader([]string{"COMPONENT", "INDEX", "PARAM", "VALUE"})
+	for _, data := range params {
+		table.Append([]string{data.Component, data.Index, data.Key, data.Value})
+	}
+
+	return table.Render()
+}

actions/param_list_test.go

@@ -17,92 +17,123 @@ package actions
 
 import (
 	"bytes"
+	"path/filepath"
 	"testing"
 
 	"github.com/ksonnet/ksonnet/component"
 	cmocks "github.com/ksonnet/ksonnet/component/mocks"
+	"github.com/ksonnet/ksonnet/metadata/app"
 	amocks "github.com/ksonnet/ksonnet/metadata/app/mocks"
-	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestParamList_with_component_name(t *testing.T) {
-	withApp(t, func(appMock *amocks.App) {
-		componentName := "deployment"
-		module := "ns"
-		envName := ""
-
-		ns := &cmocks.Module{}
-
-		c := &cmocks.Component{}
-
-		nsParams := []component.ModuleParameter{
-			{Component: "deployment", Index: "0", Key: "key", Value: `"value"`},
-		}
-		c.On("Params", "").Return(nsParams, nil)
-
-		cm := &cmocks.Manager{}
-		cm.On("Module", mock.Anything, "ns").Return(ns, nil)
-		cm.On("Component", mock.Anything, "ns", "deployment").Return(c, nil)
-
-		in := map[string]interface{}{
-			OptionApp:           appMock,
-			OptionComponentName: componentName,
-			OptionModule:        module,
-			OptionEnvName:       envName,
-		}
-
-		a, err := NewParamList(in)
-		require.NoError(t, err)
+func TestParamList(t *testing.T) {
+	moduleParams := []component.ModuleParameter{
+		{Component: "deployment", Index: "0", Key: "key", Value: `"value"`},
+	}
 
-		a.cm = cm
+	module := &cmocks.Module{}
+	module.On("Params", "envName").Return(moduleParams, nil)
+	module.On("Params", "").Return(moduleParams, nil)
 
-		var buf bytes.Buffer
-		a.out = &buf
+	c := &cmocks.Component{}
+	c.On("Params", "").Return(moduleParams, nil)
 
-		err = a.Run()
-		require.NoError(t, err)
-
-		assertOutput(t, "param_list/with_component.txt", buf.String())
-	})
-}
-
-func TestParamList_without_component_name(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		componentName := ""
-		module := "ns"
-		envName := ""
-
-		nsParams := []component.ModuleParameter{
-			{Component: "deployment", Index: "0", Key: "key1", Value: `"value"`},
-			{Component: "deployment", Index: "0", Key: "key2", Value: `"value"`},
+		cases := []struct {
+			name            string
+			in              map[string]interface{}
+			findModulesFn   func(t *testing.T) findModulesFn
+			findModuleFn    func(t *testing.T) findModuleFn
+			findComponentFn func(t *testing.T) findComponentFn
+			outputFile      string
+		}{
+			{
+				name: "component name",
+				in: map[string]interface{}{
+					OptionApp:           appMock,
+					OptionComponentName: "deployment",
+					OptionModule:        "module",
+				},
+				findModuleFn: func(t *testing.T) findModuleFn {
+					return func(a app.App, moduleName string) (component.Module, error) {
+						assert.Equal(t, "module", moduleName)
+						return module, nil
+					}
+				},
+				findComponentFn: func(t *testing.T) findComponentFn {
+					return func(a app.App, moduleName, componentName string) (component.Component, error) {
+						assert.Equal(t, "module", moduleName)
+						assert.Equal(t, "deployment", componentName)
+						return c, nil
+					}
+				},
+				outputFile: filepath.Join("param", "list", "with_component.txt"),
+			},
+			{
+				name: "no component name",
+				in: map[string]interface{}{
+					OptionApp:    appMock,
+					OptionModule: "module",
+				},
+				findModuleFn: func(t *testing.T) findModuleFn {
+					return func(a app.App, moduleName string) (component.Module, error) {
+						assert.Equal(t, "module", moduleName)
+						return module, nil
+					}
+				},
+				findComponentFn: func(t *testing.T) findComponentFn {
+					return func(a app.App, moduleName, componentName string) (component.Component, error) {
+						assert.Equal(t, "module", moduleName)
+						assert.Equal(t, "deployment", componentName)
+						return c, nil
+					}
+				},
+				outputFile: filepath.Join("param", "list", "without_component.txt"),
+			},
+			{
+				name: "env",
+				in: map[string]interface{}{
+					OptionApp:     appMock,
+					OptionEnvName: "envName",
+				},
+				findModulesFn: func(t *testing.T) findModulesFn {
+					return func(a app.App, envName string) ([]component.Module, error) {
+						assert.Equal(t, "envName", envName)
+						return []component.Module{module}, nil
+					}
+				},
+				outputFile: filepath.Join("param", "list", "env.txt"),
+			},
 		}
 
-		ns := &cmocks.Module{}
-
-		ns.On("Params", "").Return(nsParams, nil)
+		for _, tc := range cases {
+			t.Run(tc.name, func(t *testing.T) {
 
-		cm := &cmocks.Manager{}
-		cm.On("Module", mock.Anything, "ns").Return(ns, nil)
+				a, err := NewParamList(tc.in)
+				require.NoError(t, err)
 
-		in := map[string]interface{}{
-			OptionApp:           appMock,
-			OptionComponentName: componentName,
-			OptionModule:        module,
-			OptionEnvName:       envName,
-		}
+				if tc.findModulesFn != nil {
+					a.findModulesFn = tc.findModulesFn(t)
+				}
 
-		a, err := NewParamList(in)
-		require.NoError(t, err)
+				if tc.findModuleFn != nil {
+					a.findModuleFn = tc.findModuleFn(t)
+				}
 
-		a.cm = cm
+				if tc.findComponentFn != nil {
+					a.findComponentFn = tc.findComponentFn(t)
+				}
 
-		var buf bytes.Buffer
-		a.out = &buf
+				var buf bytes.Buffer
+				a.out = &buf
 
-		err = a.Run()
-		require.NoError(t, err)
+				err = a.Run()
+				require.NoError(t, err)
 
-		assertOutput(t, "param_list/without_component.txt", buf.String())
+				assertOutput(t, tc.outputFile, buf.String())
+			})
+		}
 	})
 }

actions/testdata/param/list/with_component.txt

+COMPONENT  INDEX PARAM VALUE
+=========  ===== ===== =====
+deployment 0     key   "value"

actions/testdata/param_list/without_component.txt → actions/testdata/param/list/without_component.txt

 COMPONENT  INDEX PARAM VALUE
 =========  ===== ===== =====
-deployment 0     key1  "value"
-deployment 0     key2  "value"
+deployment 0     key   "value"

component/params.go

@@ -68,6 +68,10 @@ var (
 // NOTE: It warns when it makes a change. This serves as a temporary fix until
 // ksonnet generates the correct file.
 func upgradeParams(envName, in string) string {
-	logrus.Warnf("rewriting %q environment params to not use relative paths", envName)
-	return reParamSwap.ReplaceAllLiteralString(in, `std.extVar("__ksonnet/params")`)
+	if reParamSwap.MatchString(in) {
+		logrus.Warnf("rewriting %q environment params to not use relative paths", envName)
+		return reParamSwap.ReplaceAllLiteralString(in, `std.extVar("__ksonnet/params")`)
+	}
+
+	return in
 }

pkg/params/env_globals_set.go

@@ -52,7 +52,7 @@ func (egs *EnvGlobalsSet) Set(snippet string, p params.Params) (string, error) {
 	}
 
 	if err = egs.setParams(obj, p); err != nil {
-		return "", errors.Wrap(err, "delete entry")
+		return "", errors.Wrap(err, "set global params")
 	}
 
 	// root node should be a local if not, return an error

pkg/params/env_param_set.go

@@ -57,7 +57,7 @@ func (epa *EnvParamSet) Set(componentName, snippet string, p params.Params) (str
 	}
 
 	if err = epa.setParams(obj, componentName, p); err != nil {
-		return "", errors.Wrap(err, "delete entry")
+		return "", errors.Wrap(err, "set params")
 	}
 
 	// root node should be a local if not, return an error