Merge environment-scoped libraries with environment overrides

* Simplify app code dealing with overrides
* env set will not mutate passed environment, filters out library configration for overrides
* app.Environment, app.Environments return merged views with overrides applied
* Test override environment merging
* remove redundant calls to mergedEnvironment
* Remove name parameter from AddEnvironment
* Fix override flag for merged environments
Signed-off-by: Oren Shomron <shomron@gmail.com>

pkg/actions/env_set.go

@@ -18,6 +18,7 @@ package actions
 import (
 	"github.com/ksonnet/ksonnet/pkg/app"
 	"github.com/ksonnet/ksonnet/pkg/env"
+	"github.com/pkg/errors"
 )
 
 // EnvSetNamespace is an option for setting a new namespace name.
@@ -50,7 +51,7 @@ func RunEnvSet(m map[string]interface{}) error {
 
 // func types for renaming and updating environments
 type envRenameFn func(a app.App, from, to string, override bool) error
-type updateEnvFn func(a app.App, envName, k8sAPISpec string, spec *app.EnvironmentConfig, override bool) error
+type saveFn func(a app.App, envName, k8sAPISpec string, spec *app.EnvironmentConfig, override bool) error
 
 // EnvSet sets targets for an environment.
 type EnvSet struct {
@@ -62,7 +63,7 @@ type EnvSet struct {
 	newAPISpec string
 
 	envRenameFn envRenameFn
-	updateEnvFn updateEnvFn
+	saveFn      saveFn
 }
 
 // NewEnvSet creates an instance of EnvSet.
@@ -78,7 +79,7 @@ func NewEnvSet(m map[string]interface{}) (*EnvSet, error) {
 		newAPISpec: ol.LoadOptionalString(OptionSpecFlag),
 
 		envRenameFn: env.Rename,
-		updateEnvFn: updateEnv,
+		saveFn:      save,
 	}
 
 	if ol.err != nil {
@@ -99,7 +100,7 @@ func (es *EnvSet) Run() error {
 		return err
 	}
 
-	if err := es.updateEnvConfig(env); err != nil {
+	if err := es.updateEnvConfig(*env, es.newNsName, es.newServer, es.newAPISpec, env.IsOverride()); err != nil {
 		return err
 	}
 
@@ -118,22 +119,50 @@ func (es *EnvSet) updateName(isOverride bool) error {
 	return nil
 }
 
-func (es *EnvSet) updateEnvConfig(env *app.EnvironmentConfig) error {
-	if es.newNsName == "" && es.newServer == "" && es.newAPISpec == "" {
+// updateEnvConfig merges the provided environment config with optional override settings and the  creates and saves a new environment config based on the provided
+// base configuration. It will be merged with the provided configuration settings.
+// If isOverride is specified, Libaries will be filtered out of the merged configuration, as those should always be
+// managed in the primary application config.
+func (es *EnvSet) updateEnvConfig(env app.EnvironmentConfig, namespace, server, k8sAPISpec string, isOverride bool) error {
+	if env.Name == "" {
+		return errors.Errorf("empty environment name")
+	}
+	if namespace == "" && server == "" && k8sAPISpec == "" {
+		// Nothing to update
 		return nil
 	}
 
-	if es.newNsName != "" {
-		env.Destination.Namespace = es.newNsName
+	var newEnv app.EnvironmentConfig
+	newEnv = env // this is a copy
+
+	var destination *app.EnvironmentDestinationSpec
+	if env.Destination != nil {
+		var destCopy app.EnvironmentDestinationSpec
+		destCopy = *env.Destination
+		destination = &destCopy // also a copy
+	}
+	if destination == nil && (server != "" || namespace != "") {
+		destination = &app.EnvironmentDestinationSpec{}
 	}
+	if server != "" {
+		destination.Server = server
+	}
+	if namespace != "" {
+		destination.Namespace = namespace
+	}
+
+	newEnv.Destination = destination
+	newEnv.KubernetesVersion = k8sAPISpec
+	// isOverride will be set by app.AddEnvironment
 
-	if es.newServer != "" {
-		env.Destination.Server = es.newServer
+	if isOverride {
+		// Libaries will always derive from the primary app.yaml
+		newEnv.Libraries = nil
 	}
 
-	return es.updateEnvFn(es.app, es.envName, es.newAPISpec, env, env.IsOverride())
+	return es.saveFn(es.app, newEnv.Name, k8sAPISpec, &newEnv, isOverride)
 }
 
-func updateEnv(a app.App, envName, k8sAPISpec string, env *app.EnvironmentConfig, override bool) error {
-	return a.AddEnvironment(envName, k8sAPISpec, env, override)
+func save(a app.App, envName, k8sAPISpec string, env *app.EnvironmentConfig, override bool) error {
+	return a.AddEnvironment(env, k8sAPISpec, override)
 }

pkg/actions/env_set_test.go

@@ -28,13 +28,14 @@ func TestEnvSet(t *testing.T) {
 	envName := "old_env_name"
 	newName := "new_env_name"
 	oldNamespace := "old_namespace"
-	namespace := "new_namesapce"
+	namespace := "new_namespace"
 	oldServer := "old_server"
 	server := "new_server"
 	newk8sAPISpec := "version:new_api_spec"
 
 	environmentMockFn := func(name string) *app.EnvironmentConfig {
 		return &app.EnvironmentConfig{
+			Name: name,
 			Destination: &app.EnvironmentDestinationSpec{
 				Namespace: oldNamespace,
 				Server:    oldServer,
@@ -48,7 +49,7 @@ func TestEnvSet(t *testing.T) {
 			in          map[string]interface{}
 			spec        *app.EnvironmentConfig
 			envRenameFn func(t *testing.T) envRenameFn
-			updateEnvFn func(t *testing.T) updateEnvFn
+			saveFn      func(t *testing.T) saveFn
 		}{
 			{
 				name: "rename environment",
@@ -74,14 +75,15 @@ func TestEnvSet(t *testing.T) {
 					OptionEnvName:   envName,
 					OptionNamespace: namespace,
 				},
-				updateEnvFn: func(t *testing.T) updateEnvFn {
+				saveFn: func(t *testing.T) saveFn {
 					return func(a app.App, envName, k8sAPISpec string, spec *app.EnvironmentConfig, override bool) error {
-						assert.Equal(t, spec, &app.EnvironmentConfig{
+						assert.Equal(t, &app.EnvironmentConfig{
+							Name: envName,
 							Destination: &app.EnvironmentDestinationSpec{
 								Namespace: namespace,
 								Server:    oldServer,
 							},
-						})
+						}, spec)
 						return nil
 					}
 				},
@@ -93,14 +95,15 @@ func TestEnvSet(t *testing.T) {
 					OptionEnvName: envName,
 					OptionServer:  server,
 				},
-				updateEnvFn: func(t *testing.T) updateEnvFn {
+				saveFn: func(t *testing.T) saveFn {
 					return func(a app.App, envName, k8sAPISpec string, spec *app.EnvironmentConfig, override bool) error {
-						assert.Equal(t, spec, &app.EnvironmentConfig{
+						assert.Equal(t, &app.EnvironmentConfig{
+							Name: envName,
 							Destination: &app.EnvironmentDestinationSpec{
 								Namespace: oldNamespace,
 								Server:    server,
 							},
-						})
+						}, spec)
 						return nil
 					}
 				},
@@ -112,9 +115,10 @@ func TestEnvSet(t *testing.T) {
 					OptionEnvName:  envName,
 					OptionSpecFlag: newk8sAPISpec,
 				},
-				updateEnvFn: func(t *testing.T) updateEnvFn {
+				saveFn: func(t *testing.T) saveFn {
 					return func(a app.App, envName, k8sAPISpec string, spec *app.EnvironmentConfig, override bool) error {
 						assert.Equal(t, newk8sAPISpec, k8sAPISpec)
+						assert.Equal(t, newk8sAPISpec, spec.KubernetesVersion)
 						return nil
 					}
 				},
@@ -129,14 +133,16 @@ func TestEnvSet(t *testing.T) {
 					OptionServer:     server,
 					OptionSpecFlag:   newk8sAPISpec,
 				},
-				updateEnvFn: func(t *testing.T) updateEnvFn {
+				saveFn: func(t *testing.T) saveFn {
 					return func(a app.App, newName, k8sAPISpec string, spec *app.EnvironmentConfig, override bool) error {
-						assert.Equal(t, spec, &app.EnvironmentConfig{
+						assert.Equal(t, &app.EnvironmentConfig{
+							Name: newName,
 							Destination: &app.EnvironmentDestinationSpec{
 								Namespace: namespace,
 								Server:    server,
 							},
-						})
+							KubernetesVersion: newk8sAPISpec,
+						}, spec)
 						assert.Equal(t, newk8sAPISpec, k8sAPISpec)
 						return nil
 					}
@@ -160,10 +166,20 @@ func TestEnvSet(t *testing.T) {
 
 				if tc.envRenameFn != nil {
 					a.envRenameFn = tc.envRenameFn(t)
+				} else {
+					a.envRenameFn = func(a app.App, from, to string, override bool) error {
+						t.Errorf("unexpected call: rename")
+						return nil
+					}
 				}
 
-				if tc.updateEnvFn != nil {
-					a.updateEnvFn = tc.updateEnvFn(t)
+				if tc.saveFn != nil {
+					a.saveFn = tc.saveFn(t)
+				} else {
+					a.saveFn = func(a app.App, newName, k8sAPISpec string, spec *app.EnvironmentConfig, override bool) error {
+						t.Errorf("unexpected call: save")
+						return nil
+					}
 				}
 
 				appMock.On("Environment", tc.in[OptionEnvName]).Return(environmentMockFn, nil)

pkg/app/app.go

@@ -57,7 +57,7 @@ var (
 // App is a ksonnet application.
 type App interface {
 	// AddEnvironment adds an environment.
-	AddEnvironment(name, k8sSpecFlag string, spec *EnvironmentConfig, isOverride bool) error
+	AddEnvironment(spec *EnvironmentConfig, k8sSpecFlag string, isOverride bool) error
 	// AddRegistry adds a registry.
 	AddRegistry(spec *RegistryConfig, isOverride bool) error
 	// CurrentEnvironment returns the current environment name or an empty string.

pkg/app/app001.go

@@ -54,17 +54,24 @@ func NewApp001(fs afero.Fs, root string) *App001 {
 
 // AddEnvironment adds an environment spec to the app spec. If the spec already exists,
 // it is overwritten.
-func (a *App001) AddEnvironment(name, k8sSpecFlag string, spec *EnvironmentConfig, isOverride bool) error {
+func (a *App001) AddEnvironment(e *EnvironmentConfig, k8sSpecFlag string, isOverride bool) error {
+	if e == nil {
+		return errors.Errorf("nil environment configuraion")
+	}
+	if e.Name == "" {
+		return errors.Errorf("invalid environment name")
+	}
+
 	// if it is an override, write the destination to override file. If not, do the normal thing.
 
-	envPath := filepath.Join(a.root, EnvironmentDirName, name)
+	envPath := filepath.Join(a.root, EnvironmentDirName, e.Name)
 	if err := a.fs.MkdirAll(envPath, DefaultFolderPermissions); err != nil {
 		return err
 	}
 
 	specPath := filepath.Join(envPath, app001specJSON)
 
-	b, err := json.Marshal(spec.Destination)
+	b, err := json.Marshal(e.Destination)
 	if err != nil {
 		return err
 	}
@@ -73,7 +80,7 @@ func (a *App001) AddEnvironment(name, k8sSpecFlag string, spec *EnvironmentConfi
 		return err
 	}
 
-	_, err = LibUpdater(a.fs, k8sSpecFlag, a.appLibPath(name))
+	_, err = LibUpdater(a.fs, k8sSpecFlag, a.appLibPath(e.Name))
 	return err
 }
 

pkg/app/app001_test.go

@@ -165,6 +165,7 @@ func TestApp001_Environment(t *testing.T) {
 func TestApp001_AddEnvironment(t *testing.T) {
 	withApp001Fs(t, "app001_app.yaml", func(app *App001) {
 		newEnv := &EnvironmentConfig{
+			Name: "us-west/qa",
 			Destination: &EnvironmentDestinationSpec{
 				Namespace: "some-namespace",
 				Server:    "http://example.com",
@@ -173,7 +174,7 @@ func TestApp001_AddEnvironment(t *testing.T) {
 		}
 
 		k8sSpecFlag := "version:v1.8.7"
-		err := app.AddEnvironment("us-west/qa", k8sSpecFlag, newEnv, false)
+		err := app.AddEnvironment(newEnv, k8sSpecFlag, false)
 		require.NoError(t, err)
 
 		_, err = app.Environment("us-west/qa")

pkg/app/app010.go

@@ -55,12 +55,24 @@ func NewApp010(fs afero.Fs, root string) *App010 {
 
 // AddEnvironment adds an environment spec to the app spec. If the spec already exists,
 // it is overwritten.
-func (a *App010) AddEnvironment(name, k8sSpecFlag string, newEnv *EnvironmentConfig, isOverride bool) error {
+func (a *App010) AddEnvironment(newEnv *EnvironmentConfig, k8sSpecFlag string, isOverride bool) error {
 	logrus.WithFields(logrus.Fields{
 		"k8s-spec-flag": k8sSpecFlag,
-		"name":          name,
+		"name":          newEnv.Name,
 	}).Debug("adding environment")
 
+	if newEnv == nil {
+		return errors.Errorf("nil environment")
+	}
+
+	if newEnv.Name == "" {
+		return errors.Errorf("invalid environment name")
+	}
+
+	if isOverride && len(newEnv.Libraries) > 0 {
+		return errors.Errorf("library references not allowed in overrides")
+	}
+
 	if err := a.load(); err != nil {
 		return errors.Wrap(err, "load configuration")
 	}
@@ -77,9 +89,9 @@ func (a *App010) AddEnvironment(name, k8sSpecFlag string, newEnv *EnvironmentCon
 	newEnv.isOverride = isOverride
 
 	if isOverride {
-		a.overrides.Environments[name] = newEnv
+		a.overrides.Environments[newEnv.Name] = newEnv
 	} else {
-		a.config.Environments[name] = newEnv
+		a.config.Environments[newEnv.Name] = newEnv
 	}
 
 	return a.save()
@@ -196,16 +208,17 @@ func (a *App010) RemoveEnvironment(envName string, override bool) error {
 		return errors.Wrap(err, "load configuration")
 	}
 
-	if _, ok := a.config.Environments[envName]; !ok {
-		return errors.Errorf("environment %q does not exist", envName)
+	envMap := a.config.Environments
+	if override {
+		envMap = a.overrides.Environments
 	}
 
-	if override {
-		delete(a.overrides.Environments, envName)
-	} else {
-		delete(a.config.Environments, envName)
+	if _, ok := envMap[envName]; !ok {
+		return errors.Errorf("environment %q does not exist", envName)
 	}
 
+	delete(envMap, envName)
+
 	return a.save()
 }
 
@@ -215,21 +228,17 @@ func (a *App010) RenameEnvironment(from, to string, override bool) error {
 		return errors.Wrap(err, "load configuration")
 	}
 
+	envMap := a.config.Environments
 	if override {
-		if _, ok := a.overrides.Environments[from]; !ok {
-			return errors.Errorf("environment %q does not exist", from)
-		}
-		a.overrides.Environments[to] = a.overrides.Environments[from]
-		a.overrides.Environments[to].Path = to
-		delete(a.overrides.Environments, from)
-	} else {
-		if _, ok := a.config.Environments[from]; !ok {
-			return errors.Errorf("environment %q does not exist", from)
-		}
-		a.config.Environments[to] = a.config.Environments[from]
-		a.config.Environments[to].Path = to
-		delete(a.config.Environments, from)
+		envMap = a.overrides.Environments
+	}
+
+	if _, ok := envMap[from]; !ok {
+		return errors.Errorf("environment %q does not exist", from)
 	}
+	envMap[to] = envMap[from]
+	envMap[to].Path = to
+	delete(envMap, from)
 
 	if err := moveEnvironment(a.fs, a.root, from, to); err != nil {
 		return err
@@ -247,7 +256,7 @@ func (a *App010) UpdateTargets(envName string, targets []string) error {
 
 	spec.Targets = targets
 
-	return errors.Wrap(a.AddEnvironment(envName, "", spec, spec.isOverride), "update targets")
+	return errors.Wrap(a.AddEnvironment(spec, "", spec.isOverride), "update targets")
 }
 
 // Upgrade upgrades the app to the latest apiVersion.

pkg/app/app010_test.go

@@ -34,6 +34,7 @@ func TestApp010_AddEnvironment(t *testing.T) {
 		envLen := len(envs)
 
 		newEnv := &EnvironmentConfig{
+			Name: "us-west/qa",
 			Destination: &EnvironmentDestinationSpec{
 				Namespace: "some-namespace",
 				Server:    "http://example.com",
@@ -42,7 +43,7 @@ func TestApp010_AddEnvironment(t *testing.T) {
 		}
 
 		k8sSpecFlag := "version:v1.8.7"
-		err = app.AddEnvironment("us-west/qa", k8sSpecFlag, newEnv, false)
+		err = app.AddEnvironment(newEnv, k8sSpecFlag, false)
 		require.NoError(t, err)
 
 		envs, err = app.Environments()
@@ -67,7 +68,7 @@ func TestApp010_AddEnvironment_empty_spec_flag(t *testing.T) {
 
 		env.Destination.Namespace = "updated"
 
-		err = app.AddEnvironment("default", "", env, false)
+		err = app.AddEnvironment(env, "", false)
 		require.NoError(t, err)
 
 		envs, err = app.Environments()
@@ -160,6 +161,21 @@ func TestApp010_Environment(t *testing.T) {
 	}
 }
 
+func Test_App010_Environment_returns_copy(t *testing.T) {
+	withApp010Fs(t, "app010_app.yaml", func(app *App010) {
+		e1, err := app.Environment("default")
+		require.NoError(t, err)
+
+		e1.KubernetesVersion = "v9.9.9"
+
+		e2, err := app.Environment("default")
+		require.NoError(t, err)
+		assert.False(t, e1 == e2, "expected new pointer")
+		assert.Equal(t, "v9.9.9", e1.KubernetesVersion)
+		assert.Equal(t, "v1.7.0", e2.KubernetesVersion)
+	})
+}
+
 func TestApp010_CheckUpgrade_no_legacy_environments(t *testing.T) {
 	withApp010Fs(t, "app010_app.yaml", func(app *App010) {
 		needUpgrade, err := app.CheckUpgrade()

pkg/app/base_app.go

@@ -33,10 +33,12 @@ type baseApp struct {
 	overrides *Override
 
 	mu sync.Mutex
+
+	load func() error
 }
 
 func newBaseApp(fs afero.Fs, root string) *baseApp {
-	return &baseApp{
+	ba := &baseApp{
 		fs:     fs,
 		root:   root,
 		config: &Spec{},
@@ -45,6 +47,8 @@ func newBaseApp(fs afero.Fs, root string) *baseApp {
 			Registries:   RegistryConfigs{},
 		},
 	}
+	ba.load = ba.doLoad
+	return ba
 }
 
 func (ba *baseApp) CurrentEnvironment() string {
@@ -104,7 +108,7 @@ func (ba *baseApp) save() error {
 	return nil
 }
 
-func (ba *baseApp) load() error {
+func (ba *baseApp) doLoad() error {
 	ba.mu.Lock()
 	defer ba.mu.Unlock()
 
@@ -314,34 +318,125 @@ func (ba *baseApp) Environment(name string) (*EnvironmentConfig, error) {
 		return nil, errors.Wrap(err, "load configuration")
 	}
 
-	for k, v := range ba.overrides.Environments {
-		if k == name {
-			return v, nil
-		}
+	e := ba.mergedEnvironment(name)
+	if e == nil {
+		return nil, errors.Errorf("environment %q was not found", name)
+	}
+	return e, nil
+}
+
+func deepCopyLibraries(src LibraryConfigs) LibraryConfigs {
+	if src == nil {
+		return LibraryConfigs(nil)
+	}
+
+	lc := LibraryConfigs{}
+	for k, v := range src {
+		c := *v
+		lc[k] = &c
+	}
+	return lc
+}
+
+func deepCopyEnvironmentConfig(src EnvironmentConfig) *EnvironmentConfig {
+	e := src
+
+	if src.Destination != nil {
+		d := *src.Destination
+		e.Destination = &d
+	}
+	if src.Targets != nil {
+		t := make([]string, len(src.Targets))
+		copy(t, src.Targets)
+		e.Targets = t
 	}
+	if src.Libraries != nil {
+		e.Libraries = deepCopyLibraries(src.Libraries)
+	}
+
+	return &e
+}
+
+// mergedEnvrionment returns a fresh copy of the named environment, merged with
+// optional overrides if present. Note overrides cannot override environment-scoped library
+// references.
+// Returns nil if the envrionment is not present and non-nil in either primary configuration
+// or overrides.
+func (ba *baseApp) mergedEnvironment(name string) *EnvironmentConfig {
+	var hasPrimary, hasOverride bool
+	var primary, override *EnvironmentConfig
 
-	for k, v := range ba.config.Environments {
-		if k == name {
-			return v, nil
+	if ba.config != nil {
+		primary, hasPrimary = ba.config.Environments[name]
+		if primary == nil {
+			hasPrimary = false
+		}
+	}
+	if ba.overrides != nil {
+		override, hasOverride = ba.overrides.Environments[name]
+		if override == nil {
+			hasOverride = false
 		}
 	}
 
-	return nil, errors.Errorf("environment %q was not found", name)
+	switch {
+	case hasPrimary && !hasOverride:
+		e := deepCopyEnvironmentConfig(*primary)
+		e.isOverride = false
+		return e
+	case hasPrimary && hasOverride:
+		combined := deepCopyEnvironmentConfig(*primary)
+		combined.Name = override.Name
+		combined.KubernetesVersion = override.KubernetesVersion
+		combined.Path = override.Path
+		if override.Destination != nil {
+			d := *override.Destination
+			combined.Destination = &d
+		}
+		if override.Targets != nil {
+			t := make([]string, len(override.Targets))
+			copy(t, override.Targets)
+			combined.Targets = t
+		}
+		combined.isOverride = true
+		return combined
+	case hasOverride:
+		e := deepCopyEnvironmentConfig(*override)
+		e.isOverride = true
+		return e
+	default:
+		return nil
+	}
 }
 
-// Environments returns all environment specs.
+// Environments returns all environment specs, merged with any corresponding overrides.
+// Note overrides cannot override environment libraries.
 func (ba *baseApp) Environments() (EnvironmentConfigs, error) {
 	if err := ba.load(); err != nil {
 		return nil, errors.Wrap(err, "load configuration")
 	}
 
+	// Build merged list of keys
 	environments := EnvironmentConfigs{}
-	for k, v := range ba.config.Environments {
-		environments[k] = v
+	if ba.config != nil {
+		for k := range ba.config.Environments {
+			environments[k] = nil
+		}
 	}
+	if ba.overrides != nil {
+		for k := range ba.overrides.Environments {
+			environments[k] = nil
+		}
+	}
+
+	for k := range environments {
+		e := ba.mergedEnvironment(k)
+		if e == nil {
+			delete(environments, k)
+			continue
+		}
 
-	for k, v := range ba.overrides.Environments {
-		environments[k] = v
+		environments[k] = e
 	}
 
 	return environments, nil

pkg/app/base_app_test.go

@@ -261,3 +261,93 @@ func Test_baseApp_load_override_invalid(t *testing.T) {
 	err := ba.load()
 	require.Error(t, err)
 }
+
+func Test_baseApp_environment_override_is_merged(t *testing.T) {
+	fs := afero.NewMemMapFs()
+	ba := newBaseApp(fs, "/")
+	ba.load = func() error {
+		return nil
+	}