Refactor override handling

* baseApp.load() / baseApp.save() are override-aware
* app.read() / app.write() (schema.go) are not - they only serialize/deserialize app.yaml
* baseApp.load() / baseApp.save() now call app.read() / app.write() instead of duplicating serialization logic
* Removed isOverride flag from EnvironmentConfig, RegistryConfig
* Removed override logic from app.Load() - this is handled in baseApp.load() now
* env set command now respects the --override flag to indicate where to write changes

Closes #830
Signed-off-by: Oren Shomron <shomron@gmail.com>

pkg/actions/actions_test.go

@@ -278,12 +278,11 @@ func mockNsWithName(name string) *cmocks.Module {
 	return m
 }
 
-func mockRegistry(name string, isOverride bool) *rmocks.Registry {
+func mockRegistry(name string) *rmocks.Registry {
 	m := &rmocks.Registry{}
 	m.On("Name").Return(name)
 	m.On("Protocol").Return(registry.ProtocolGitHub)
 	m.On("URI").Return("github.com/ksonnet/parts/tree/master/incubator")
-	m.On("IsOverride").Return(isOverride)
 
 	return m
 }

pkg/actions/env_list.go

@@ -38,9 +38,10 @@ func RunEnvList(m map[string]interface{}) error {
 // EnvList lists available namespaces. To initialize EnvList,
 // use the `NewEnvList` constructor.
 type EnvList struct {
-	envListFn  func() (app.EnvironmentConfigs, error)
-	outputType string
-	out        io.Writer
+	envListFn       func() (app.EnvironmentConfigs, error)
+	envIsOverrideFn func(name string) bool
+	outputType      string
+	out             io.Writer
 }
 
 // NewEnvList creates an instance of EnvList
@@ -55,9 +56,10 @@ func NewEnvList(m map[string]interface{}) (*EnvList, error) {
 	}
 
 	el := &EnvList{
-		outputType: outputType,
-		envListFn:  a.Environments,
-		out:        os.Stdout,
+		outputType:      outputType,
+		envListFn:       a.Environments,
+		envIsOverrideFn: a.IsEnvOverride,
+		out:             os.Stdout,
 	}
 
 	return el, nil
@@ -83,7 +85,7 @@ func (el *EnvList) Run() error {
 
 	for name, env := range environments {
 		override := ""
-		if env.IsOverride() {
+		if el.envIsOverrideFn(name) {
 			override = "*"
 		}
 

pkg/actions/env_list_test.go

@@ -24,6 +24,7 @@ import (
 	amocks "github.com/ksonnet/ksonnet/pkg/app/mocks"
 	"github.com/ksonnet/ksonnet/pkg/util/test"
 	"github.com/pkg/errors"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
@@ -52,10 +53,12 @@ func TestEnvList(t *testing.T) {
 		}
 
 		appMock.On("Environments").Return(envs, nil)
+		appMock.On("IsEnvOverride", mock.Anything).Return(false)
 	}
 
 	envListFail := func(appMock *amocks.App) {
 		appMock.On("Environments").Return(nil, errors.New("failed"))
+		appMock.On("IsEnvOverride", mock.Anything).Return(false)
 	}
 
 	cases := []struct {

pkg/actions/env_set.go

@@ -61,6 +61,7 @@ type EnvSet struct {
 	newNsName  string
 	newServer  string
 	newAPISpec string
+	isOverride bool
 
 	envRenameFn envRenameFn
 	saveFn      saveFn
@@ -77,6 +78,7 @@ func NewEnvSet(m map[string]interface{}) (*EnvSet, error) {
 		newNsName:  ol.LoadOptionalString(OptionNamespace),
 		newServer:  ol.LoadOptionalString(OptionServer),
 		newAPISpec: ol.LoadOptionalString(OptionSpecFlag),
+		isOverride: ol.LoadOptionalBool(OptionOverride),
 
 		envRenameFn: env.Rename,
 		saveFn:      save,
@@ -96,11 +98,11 @@ func (es *EnvSet) Run() error {
 		return err
 	}
 
-	if err := es.updateName(env.IsOverride()); err != nil {
+	if err := es.updateName(es.isOverride); err != nil {
 		return err
 	}
 
-	if err := es.updateEnvConfig(*env, es.newNsName, es.newServer, es.newAPISpec, env.IsOverride()); err != nil {
+	if err := es.updateEnvConfig(*env, es.newNsName, es.newServer, es.newAPISpec, es.isOverride); err != nil {
 		return err
 	}
 

pkg/actions/env_set_test.go

@@ -155,6 +155,7 @@ func TestEnvSet(t *testing.T) {
 					}
 				},
 			},
+			// TODO add tests for overrides here
 		}
 
 		for _, tc := range cases {

pkg/actions/registry_list.go

@@ -41,8 +41,9 @@ type RegistryList struct {
 	app        app.App
 	outputType string
 
-	registryListFn func(ksApp app.App) ([]registry.Registry, error)
-	out            io.Writer
+	registryListFn       func(ksApp app.App) ([]registry.Registry, error)
+	registryIsOverrideFn func(name string) bool
+	out                  io.Writer
 }
 
 // NewRegistryList creates an instance of RegistryList
@@ -50,14 +51,20 @@ func NewRegistryList(m map[string]interface{}) (*RegistryList, error) {
 	ol := newOptionLoader(m)
 
 	httpClient := ol.LoadHTTPClient()
+	a := ol.LoadApp()
+	if ol.err != nil {
+		return nil, ol.err
+	}
+
 	rl := &RegistryList{
-		app:        ol.LoadApp(),
+		app:        a,
 		outputType: ol.LoadOptionalString(OptionOutput),
 
 		registryListFn: func(ksApp app.App) ([]registry.Registry, error) {
 			return registry.List(ksApp, httpClient)
 		},
-		out: os.Stdout,
+		registryIsOverrideFn: a.IsRegistryOverride,
+		out:                  os.Stdout,
 	}
 
 	if ol.err != nil {
@@ -87,7 +94,7 @@ func (rl *RegistryList) Run() error {
 
 	for _, r := range registries {
 		override := ""
-		if r.IsOverride() {
+		if rl.registryIsOverrideFn(r.Name()) {
 			override = "*"
 		}
 

pkg/actions/registry_list_test.go

@@ -66,11 +66,17 @@ func TestRegistryList(t *testing.T) {
 
 				a.registryListFn = func(app.App) ([]registry.Registry, error) {
 					registries := []registry.Registry{
-						mockRegistry("override", true),
-						mockRegistry("incubator", false),
+						mockRegistry("override"),
+						mockRegistry("incubator"),
 					}
 					return registries, nil
 				}
+				a.registryIsOverrideFn = func(name string) bool {
+					if name == "override" {
+						return true
+					}
+					return false
+				}
 
 				err = a.Run()
 				if tc.isErr {

pkg/app/app.go

@@ -68,6 +68,10 @@ type App interface {
 	Fs() afero.Fs
 	// HTTPClient is the app's http client
 	HTTPClient() *http.Client
+	// IsEnvOverride returns whether the specified environment has overriding configuration
+	IsEnvOverride(name string) bool
+	// IsRegistryOverride returns whether the specified registry has overriding configuration
+	IsRegistryOverride(name string) bool
 	// LibPath returns the path of the lib for an environment.
 	LibPath(envName string) (string, error)
 	// Libraries returns all environments.
@@ -100,17 +104,23 @@ type App interface {
 
 // Load loads the application configuration.
 func Load(fs afero.Fs, httpClient *http.Client, appRoot string) (App, error) {
-	spec, err := read(fs, appRoot)
+	if fs == nil {
+		return nil, errors.New("nil fs interface")
+	}
+
+	_, err := fs.Stat(specPath(appRoot))
 	if os.IsNotExist(err) {
 		// During `ks init`, app.yaml will not yet exist - generate a new one.
 		return NewBaseApp(fs, appRoot, httpClient), nil
 	}
 	if err != nil {
-		return nil, errors.Wrap(err, "reading app configuration")
+		return nil, errors.Wrap(err, "checking existence of app configuration")
 	}
 
 	a := NewBaseApp(fs, appRoot, httpClient)
-	a.config = spec
+	if err := a.doLoad(); err != nil {
+		return nil, errors.Wrap(err, "reading app configuration")
+	}
 
 	return a, nil
 }

pkg/app/app_test.go

@@ -420,3 +420,68 @@ func withAppFs(t *testing.T, appName string, fn func(app *baseApp)) {
 
 	fn(app)
 }
+
+func TestApp_Load(t *testing.T) {
+	fs := afero.NewMemMapFs()
+
+	expectedEnvs := EnvironmentConfigs{
+		"default": &EnvironmentConfig{
+			Name: "default",
+			Destination: &EnvironmentDestinationSpec{
+				Namespace: "some-namespace",
+				Server:    "http://example.com",
+			},
+			KubernetesVersion: "v1.7.0",
+			Path:              "default",
+		},
+		"us-east/test": &EnvironmentConfig{
+			Name: "us-east/test",
+			Destination: &EnvironmentDestinationSpec{
+				Namespace: "some-namespace",
+				Server:    "http://example.com",
+			},
+			KubernetesVersion: "v1.7.0",
+			Path:              "us-east/test",
+		},
+		"us-west/prod": &EnvironmentConfig{
+			Name: "us-west/prod",
+			Destination: &EnvironmentDestinationSpec{
+				Namespace: "some-namespace",
+				Server:    "http://example.com",
+			},
+			KubernetesVersion: "v1.7.0",
+			Path:              "us-west/prod",
+		},
+		"us-west/test": &EnvironmentConfig{
+			Name: "us-west/test",
+			Destination: &EnvironmentDestinationSpec{
+				Namespace: "some-namespace",
+				Server:    "http://example.com",
+			},
+			KubernetesVersion: "v1.7.0",
+			Path:              "us-west/test",
+		},
+	}
+
+	stageFile(t, fs, "app030_app.yaml", "/app.yaml")
+
+	a, err := Load(fs, nil, "/")
+	require.NoError(t, err)
+
+	envs, err := a.Environments()
+	require.NoError(t, err, "loading environments")
+
+	assert.Equal(t, expectedEnvs, envs, "unexpected app content")
+}
+
+// Tests that a new app is initialized when an app.yaml does not yet exist
+func TestApp_Load_no_cfg(t *testing.T) {
+	fs := afero.NewMemMapFs()
+
+	a, err := Load(fs, nil, "/")
+	require.NoError(t, err)
+	require.NotNil(t, a)
+
+	_, err = fs.Stat("/app.yaml")
+	require.True(t, os.IsNotExist(err), "app.yaml should not exist")
+}

pkg/app/base_app.go

@@ -152,22 +152,16 @@ func (ba *baseApp) save() error {
 	// Signal we have converted to new app version
 	ba.config.APIVersion = DefaultAPIVersion
 	log.Debugf("saving app version %v", ba.config.APIVersion)
-
-	configData, err := yaml.Marshal(ba.config)
-	if err != nil {
-		return errors.Wrap(err, "convert application configuration to YAML")
-	}
-
-	if err = afero.WriteFile(ba.fs, ba.configPath(), configData, DefaultFilePermissions); err != nil {
-		return errors.Wrapf(err, "write %s", ba.configPath())
+	if err := write(ba.fs, ba.root, ba.config); err != nil {
+		return errors.Wrap(err, "serializing configuration")
 	}
 
-	if err = removeOverride(ba.fs, ba.root); err != nil {
+	if err := removeOverride(ba.fs, ba.root); err != nil {
 		return errors.Wrap(err, "clean overrides")
 	}
 
 	if ba.overrides.IsDefined() {
-		return SaveOverride(defaultYAMLEncoder, ba.fs, ba.root, ba.overrides)
+		return saveOverride(defaultYAMLEncoder, ba.fs, ba.root, ba.overrides)
 	}
 
 	return nil
@@ -177,46 +171,22 @@ func (ba *baseApp) doLoad() error {
 	ba.mu.Lock()
 	defer ba.mu.Unlock()
 
-	// TODO remove applyOverrides from read()? Remove read altogether?
 	config, err := read(ba.fs, ba.root)
 	if err != nil {
 		return err
 	}
 
-	exists, err := afero.Exists(ba.fs, ba.overridePath())
+	overrides, err := readOverrides(ba.fs, ba.root)
 	if err != nil {
-		return err
-	}
-
-	override := Override{
-		Environments: EnvironmentConfigs{},
-		Registries:   RegistryConfigs{},
+		return errors.Wrap(err, "reading app overrides")
 	}
-	if exists {
-		overrideData, err := afero.ReadFile(ba.fs, ba.overridePath())
-		if err != nil {
-			return errors.Wrapf(err, "read %s", ba.overridePath())
-		}
-		if err = yaml.Unmarshal(overrideData, &override); err != nil {
-			return errors.Wrapf(err, "unmarshal override YAML config")
-		}
-
-		if err = override.Validate(); err != nil {
-			return errors.Wrap(err, "validating override")
-		}
-
-		for k := range override.Registries {
-			override.Registries[k].isOverride = true
-		}
-
-		for k := range override.Environments {
-			override.Environments[k].isOverride = true
-		}
 
+	if overrides == nil {
+		overrides = newOverride()
 	}
 
-	ba.overrides = &override
 	ba.config = config
+	ba.overrides = overrides
 	ba.loaded = true
 
 	return nil
@@ -231,25 +201,20 @@ func (ba *baseApp) AddRegistry(newReg *RegistryConfig, isOverride bool) error {
 		return ErrRegistryNameInvalid
 	}
 
+	var regMap = ba.config.Registries
 	if isOverride {
-		_, exists := ba.overrides.Registries[newReg.Name]
-		if exists {
-			return ErrRegistryExists
+		if ba.overrides == nil {
+			ba.overrides = newOverride()
 		}
-
-		newReg.isOverride = true
-
-		ba.overrides.Registries[newReg.Name] = newReg
-		return ba.save()
+		regMap = ba.overrides.Registries
 	}
 
-	_, exists := ba.config.Registries[newReg.Name]
+	_, exists := regMap[newReg.Name]
 	if exists {
 		return ErrRegistryExists
 	}
 
-	newReg.isOverride = false
-	ba.config.Registries[newReg.Name] = newReg
+	regMap[newReg.Name] = newReg
 
 	return ba.save()
 }
@@ -487,7 +452,6 @@ func (ba *baseApp) mergedEnvironment(name string) *EnvironmentConfig {
 	switch {
 	case hasPrimary && !hasOverride:
 		e := deepCopyEnvironmentConfig(*primary)
-		e.isOverride = false
 		return e
 	case hasPrimary && hasOverride:
 		combined := deepCopyEnvironmentConfig(*primary)
@@ -503,11 +467,9 @@ func (ba *baseApp) mergedEnvironment(name string) *EnvironmentConfig {
 			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
@@ -582,14 +544,15 @@ func (ba *baseApp) AddEnvironment(newEnv *EnvironmentConfig, k8sSpecFlag string,
 		newEnv.KubernetesVersion = ver
 	}
 
-	newEnv.isOverride = isOverride
-
+	var envMap = ba.config.Environments
 	if isOverride {
-		ba.overrides.Environments[newEnv.Name] = newEnv
-	} else {
-		ba.config.Environments[newEnv.Name] = newEnv
+		if ba.overrides == nil {
+			ba.overrides = newOverride()
+		}
+		envMap = ba.overrides.Environments
 	}
 
+	envMap[newEnv.Name] = newEnv
 	return ba.save()
 }
 
@@ -679,7 +642,8 @@ func (ba *baseApp) UpdateTargets(envName string, targets []string) error {
 
 	spec.Targets = targets
 
-	return errors.Wrap(ba.AddEnvironment(spec, "", spec.isOverride), "update targets")
+	isOverride := ba.IsEnvOverride(envName)
+	return errors.Wrap(ba.AddEnvironment(spec, "", isOverride), "update targets")
 }
 
 // LibPath returns the lib path for an env environment.
@@ -757,3 +721,21 @@ func (ba *baseApp) Upgrade(bool) error {
 
 	return nil
 }
+
+// IsEnvOverride returns whether the specified environment has overriding configuration
+func (ba *baseApp) IsEnvOverride(name string) bool {
+	if ba.overrides == nil {
+		return false
+	}
+	_, ok := ba.overrides.Environments[name]
+	return ok
+}
+
+// IsRegistryOverride returns whether the specified registry has overriding configuration
+func (ba *baseApp) IsRegistryOverride(name string) bool {
+	if ba.overrides == nil {
+		return false
+	}
+	_, ok := ba.overrides.Registries[name]
+	return ok
+}

pkg/app/base_app_test.go

@@ -468,9 +468,8 @@ func Test_baseApp_environment_override_is_merged(t *testing.T) {
 			Server:    "http://override.com",
 			Namespace: "override",
 		},
-		Path:       "overrides/path",
-		Targets:    []string{"override1", "override2"},
-		isOverride: true,
+		Path:    "overrides/path",
+		Targets: []string{"override1", "override2"},
 	}
 
 	e, err := ba.Environment("default")
@@ -490,9 +489,8 @@ func Test_baseApp_environment_just_override(t *testing.T) {
 			Server:    "http://override.com",
 			Namespace: "override",
 		},
-		Path:       "overrides/path",
-		Targets:    []string{"override1", "override2"},
-		isOverride: false,
+		Path:    "overrides/path",
+		Targets: []string{"override1", "override2"},
 	}
 
 	expected := &EnvironmentConfig{
@@ -502,9 +500,8 @@ func Test_baseApp_environment_just_override(t *testing.T) {
 			Server:    "http://override.com",
 			Namespace: "override",
 		},
-		Path:       "overrides/path",
-		Targets:    []string{"override1", "override2"},
-		isOverride: true,
+		Path:    "overrides/path",
+		Targets: []string{"override1", "override2"},
 	}
 
 	e, err := ba.Environment("default")

pkg/app/migrations.go

@@ -310,10 +310,9 @@ func migrateSchema020To030(src *Spec020) (*Spec030, error) {
 	dst.Registries = RegistryConfigs030{}
 	for k, v := range src.Registries {
 		dst.Registries[k] = &RegistryConfig030{
-			Name:       v.Name,
-			Protocol:   v.Protocol,
-			URI:        v.URI,
-			isOverride: false,
+			Name:     v.Name,
+			Protocol: v.Protocol,
+			URI:      v.URI,
 		}
 	}
 
@@ -329,7 +328,6 @@ func migrateSchema020To030(src *Spec020) (*Spec030, error) {
 			Path:              v.Path,
 			Targets:           targets,
 			Libraries:         libs,
-			isOverride:        false,
 		}
 
 		if v.Destination != nil {

pkg/app/mocks/App.go

@@ -167,6 +167,34 @@ func (_m *App) HTTPClient() *http.Client {
 	return r0
 }
 
+// IsEnvOverride provides a mock function with given fields: name
+func (_m *App) IsEnvOverride(name string) bool {
+	ret := _m.Called(name)
+
+	var r0 bool
+	if rf, ok := ret.Get(0).(func(string) bool); ok {
+		r0 = rf(name)
+	} else {
+		r0 = ret.Get(0).(bool)
+	}
+
+	return r0
+}
+
+// IsRegistryOverride provides a mock function with given fields: name
+func (_m *App) IsRegistryOverride(name string) bool {
+	ret := _m.Called(name)
+
+	var r0 bool
+	if rf, ok := ret.Get(0).(func(string) bool); ok {
+		r0 = rf(name)
+	} else {
+		r0 = ret.Get(0).(bool)
+	}
+
+	return r0
+}
+
 // LibPath provides a mock function with given fields: envName
 func (_m *App) LibPath(envName string) (string, error) {
 	ret := _m.Called(envName)

pkg/app/override.go

@@ -17,8 +17,11 @@ package app
 
 import (
 	"os"
+	"path/filepath"
 
+	"github.com/ghodss/yaml"
 	"github.com/pkg/errors"
+	log "github.com/sirupsen/logrus"
 	"github.com/spf13/afero"
 )
 
@@ -32,8 +35,56 @@ const (
 // Override defines overrides to ksonnet project configurations.
 type Override = Override030
 
-// SaveOverride saves the override to the filesystem.
-func SaveOverride(encoder Encoder, fs afero.Fs, root string, o *Override) error {
+// overridePath constructs a path for app.override.yaml
+func overridePath(appRoot string) string {
+	return filepath.Join(appRoot, overrideYamlName)
+}
+
+func newOverride() *Override {
+	o := &Override{
+		APIVersion:   overrideVersion,
+		Kind:         overrideKind,
+		Environments: EnvironmentConfigs{},
+		Registries:   RegistryConfigs{},
+	}
+	return o
+}
+
+// readOverrides returns optional override configuration
+// Returns nil if no overrides were defined.
+func readOverrides(fs afero.Fs, root string) (*Override, error) {
+	log.Debugf("loading overrides from %s", root)
+
+	exists, err := afero.Exists(fs, overridePath(root))
+	if err != nil {
+		return nil, err
+	}
+
+	if !exists {
+		return nil, nil
+	}
+
+	var o Override
+
+	overrideConfig, err := afero.ReadFile(fs, overridePath(root))
+	if err != nil {
+		return nil, err
+	}
+
+	err = yaml.Unmarshal(overrideConfig, &o)
+	if err != nil {
+		return nil, err
+	}