Allow installing packages into environment scope

* Add --env flag to pkg install cli
* Add support for environment qualifier in app.UpdateLib()
* Refactor CacheDependency to only focus on vendoring, app update is handled by the caller
* Move default app.Environment[s]() implementation to baseApp.

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

pkg/actions/pkg_install.go

@@ -21,8 +21,9 @@ import (
 	"github.com/ksonnet/ksonnet/pkg/registry"
 )
 
-// DepCacher is a function that caches a dependency.j
-type DepCacher func(app.App, registry.InstalledChecker, pkg.Descriptor, string) error
+type libCacher func(app.App, registry.InstalledChecker, pkg.Descriptor, string) (*app.LibraryConfig, error)
+
+type libUpdater func(name string, env string, spec *app.LibraryConfig) error
 
 // RunPkgInstall runs `pkg install`
 func RunPkgInstall(m map[string]interface{}) error {
@@ -39,8 +40,10 @@ type PkgInstall struct {
 	app         app.App
 	libName     string
 	customName  string
+	envName     string
 	checker     registry.InstalledChecker
-	depCacherFn DepCacher
+	libCacherFn libCacher
+	libUpdateFn libUpdater
 }
 
 // NewPkgInstall creates an instance of PkgInstall.
@@ -56,9 +59,11 @@ func NewPkgInstall(m map[string]interface{}) (*PkgInstall, error) {
 		app:        a,
 		libName:    ol.LoadString(OptionLibName),
 		customName: ol.LoadString(OptionName),
+		envName:    ol.LoadOptionalString(OptionEnvName),
 		checker:    registry.NewPackageManager(a),
 
-		depCacherFn: registry.CacheDependency,
+		libCacherFn: registry.CacheDependency,
+		libUpdateFn: a.UpdateLib,
 	}
 
 	if ol.err != nil {
@@ -75,7 +80,17 @@ func (pi *PkgInstall) Run() error {
 		return err
 	}
 
-	return pi.depCacherFn(pi.app, pi.checker, d, customName)
+	libCfg, err := pi.libCacherFn(pi.app, pi.checker, d, customName)
+	if err != nil {
+		return err
+	}
+
+	err = pi.libUpdateFn(d.Name, pi.envName, libCfg)
+	if err != nil {
+		return err
+	}
+
+	return nil
 }
 
 func (pi *PkgInstall) parseDepSpec() (pkg.Descriptor, string, error) {

pkg/actions/pkg_install_test.go

@@ -22,6 +22,7 @@ import (
 	amocks "github.com/ksonnet/ksonnet/pkg/app/mocks"
 	"github.com/ksonnet/ksonnet/pkg/pkg"
 	"github.com/ksonnet/ksonnet/pkg/registry"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -30,16 +31,6 @@ func TestPkgInstall(t *testing.T) {
 		libName := "incubator/apache"
 		customName := "customName"
 
-		dc := func(a app.App, checker registry.InstalledChecker, d pkg.Descriptor, cn string) error {
-			expectedD := pkg.Descriptor{
-				Registry: "incubator",
-				Name:     "apache",
-			}
-			require.Equal(t, expectedD, d)
-			require.Equal(t, "customName", cn)
-			return nil
-		}
-
 		in := map[string]interface{}{
 			OptionApp:     appMock,
 			OptionLibName: libName,
@@ -49,10 +40,40 @@ func TestPkgInstall(t *testing.T) {
 		a, err := NewPkgInstall(in)
 		require.NoError(t, err)
 
-		a.depCacherFn = dc
+		newLibCfg := &app.LibraryConfig{
+			Registry: "incubator",
+			Name:     "apache",
+		}
+		expectedD := pkg.Descriptor{
+			Registry: "incubator",
+			Name:     "apache",
+		}
+
+		var cacherCalled bool
+		fakeCacher := func(a app.App, checker registry.InstalledChecker, d pkg.Descriptor, cn string) (*app.LibraryConfig, error) {
+			cacherCalled = true
+			require.Equal(t, expectedD, d)
+			require.Equal(t, "customName", cn)
+			return newLibCfg, nil
+		}
+
+		var updaterCalled bool
+		fakeUpdater := func(name string, env string, spec *app.LibraryConfig) error {
+			updaterCalled = true
+			assert.Equal(t, newLibCfg.Name, name, "unexpected library name")
+			assert.Equal(t, a.envName, env, "unexpected environment name")
+			assert.Equal(t, newLibCfg, spec, "unexpected library configuration object")
+			if spec != nil {
+				assert.Equal(t, expectedD.Name, spec.Name, "unexpected library name in configuration object")
+			}
+			return nil
+		}
+
+		a.libCacherFn = fakeCacher
+		a.libUpdateFn = fakeUpdater
 
-		libaries := app.LibraryConfigs{}
-		appMock.On("Libraries").Return(libaries, nil)
+		libraries := app.LibraryConfigs{}
+		appMock.On("Libraries").Return(libraries, nil)
 
 		registries := app.RegistryConfigs{
 			"incubator": &app.RegistryConfig{
@@ -64,6 +85,8 @@ func TestPkgInstall(t *testing.T) {
 
 		err = a.Run()
 		require.NoError(t, err)
+		assert.True(t, cacherCalled, "dependency cacher not called")
+		assert.True(t, updaterCalled, "library reference updater not called")
 	})
 }
 

pkg/app/app.go

@@ -88,8 +88,10 @@ type App interface {
 	SetCurrentEnvironment(name string) error
 	// UpdateTargets sets the targets for an environment.
 	UpdateTargets(envName string, targets []string) error
-	// UpdateLib updates a library.
-	UpdateLib(name string, spec *LibraryConfig) error
+	// UpdateLib adds or updates a library reference.
+	// env is optional - if provided the reference is scoped under the environment,
+	// otherwise it is globally scoped.
+	UpdateLib(name string, env string, spec *LibraryConfig) error
 	// UpdateRegistry updates a registry.
 	UpdateRegistry(spec *RegistryConfig) error
 	// Upgrade upgrades an application to the current version.

pkg/app/app010.go

@@ -85,45 +85,6 @@ func (a *App010) AddEnvironment(name, k8sSpecFlag string, newEnv *EnvironmentCon
 	return a.save()
 }
 
-// Environment returns the spec for an environment.
-func (a *App010) Environment(name string) (*EnvironmentConfig, error) {
-	if err := a.load(); err != nil {
-		return nil, errors.Wrap(err, "load configuration")
-	}
-
-	for k, v := range a.overrides.Environments {
-		if k == name {
-			return v, nil
-		}
-	}
-
-	for k, v := range a.config.Environments {
-		if k == name {
-			return v, nil
-		}
-	}
-
-	return nil, errors.Errorf("environment %q was not found", name)
-}
-
-// Environments returns all environment specs.
-func (a *App010) Environments() (EnvironmentConfigs, error) {
-	if err := a.load(); err != nil {
-		return nil, errors.Wrap(err, "load configuration")
-	}
-
-	environments := EnvironmentConfigs{}
-	for k, v := range a.config.Environments {
-		environments[k] = v
-	}
-
-	for k, v := range a.overrides.Environments {
-		environments[k] = v
-	}
-
-	return environments, nil
-}
-
 // CheckUpgrade initializes the App.
 func (a *App010) CheckUpgrade() (bool, error) {
 	if a == nil {

pkg/app/base_app.go

@@ -204,12 +204,46 @@ func (ba *baseApp) AddRegistry(newReg *RegistryConfig, isOverride bool) error {
 	return ba.save()
 }
 
-func (ba *baseApp) UpdateLib(name string, libSpec *LibraryConfig) error {
+// UpdateLib adds or updates a library reference.
+// env is optional - if provided the reference is scoped under the environment,
+// otherwise it is globally scoped.
+func (ba *baseApp) UpdateLib(name string, env string, libSpec *LibraryConfig) error {
 	if err := ba.load(); err != nil {
 		return errors.Wrap(err, "load configuration")
 	}
 
-	ba.config.Libraries[name] = libSpec
+	if ba.config == nil {
+		return errors.Errorf("invalid app - configuration is nil")
+	}
+
+	if libSpec.Name != name {
+		return errors.Errorf("library name mismatch: %v vs %v", libSpec.Name, name)
+	}
+
+	// TODO support app overrides
+
+	switch env {
+	case "":
+		// Globally scoped
+		ba.config.Libraries[name] = libSpec
+	default:
+		// Scoped by environment
+		e, ok := ba.config.GetEnvironmentConfig(env)
+		if !ok {
+			return errors.Errorf("invalid environment: %v", env)
+		}
+
+		if e.Libraries == nil {
+			// We may want to move this into EnvrionmentConfig unmarshaling code.
+			e.Libraries = LibraryConfigs{}
+		}
+		e.Libraries[name] = libSpec
+
+		if err := ba.config.UpdateEnvironmentConfig(env, e); err != nil {
+			return errors.Wrapf(err, "updating environment %v", env)
+		}
+	}
+
 	return ba.save()
 }
 
@@ -273,3 +307,42 @@ func (ba *baseApp) EnvironmentParams(envName string) (string, error) {
 func (ba *baseApp) VendorPath() string {
 	return filepath.Join(ba.Root(), "vendor")
 }
+
+// Environment returns the spec for an environment.
+func (ba *baseApp) Environment(name string) (*EnvironmentConfig, error) {
+	if err := ba.load(); err != nil {
+		return nil, errors.Wrap(err, "load configuration")
+	}
+
+	for k, v := range ba.overrides.Environments {
+		if k == name {
+			return v, nil
+		}
+	}
+
+	for k, v := range ba.config.Environments {
+		if k == name {
+			return v, nil
+		}
+	}
+
+	return nil, errors.Errorf("environment %q was not found", name)
+}
+
+// Environments returns all environment specs.
+func (ba *baseApp) Environments() (EnvironmentConfigs, error) {
+	if err := ba.load(); err != nil {
+		return nil, errors.Wrap(err, "load configuration")
+	}
+
+	environments := EnvironmentConfigs{}
+	for k, v := range ba.config.Environments {
+		environments[k] = v
+	}
+
+	for k, v := range ba.overrides.Environments {
+		environments[k] = v
+	}
+
+	return environments, nil
+}

pkg/app/base_app_test.go

@@ -159,7 +159,78 @@ func Test_baseApp_UpdateRegistry(t *testing.T) {
 		if tc.expectFilePath != "" {
 			assertContents(t, fs, tc.expectFilePath, ba.configPath())
 		}
-		//assertNotExists(t, fs, ba.overridePath())
+	}
+
+}
+
+func Test_baseApp_UpdateLibrary(t *testing.T) {
+	//func (ba *baseApp) UpdateLib(name string, env string, libSpec *LibraryConfig) error
+
+	tests := []struct {
+		name           string
+		libCfg         LibraryConfig
+		env            string
+		appFilePath    string
+		expectFilePath string
+		expectErr      bool
+	}{
+		{
+			name: "no such environment",
+			libCfg: LibraryConfig{
+				Name:     "nginx",
+				Registry: "incubator",
+				Version:  "1.2.3",
+			},
+			env:         "no-such-environment",
+			appFilePath: "app020_app.yaml",
+			expectErr:   true,
+		},
+		{
+			name: "success - global scope",
+			libCfg: LibraryConfig{
+				Name:     "nginx",
+				Registry: "incubator",
+				Version:  "1.2.3",
+			},
+			env:            "",
+			appFilePath:    "app020_app.yaml",
+			expectFilePath: "pkg-install-global.yaml",
+			expectErr:      false,
+		},
+		{
+			name: "success - environment scope",
+			libCfg: LibraryConfig{
+				Name:     "nginx",
+				Registry: "incubator",
+				Version:  "1.2.3",
+			},
+			env:            "default",
+			appFilePath:    "app010_app.yaml",
+			expectFilePath: "pkg-install-env-scope.yaml",
+			expectErr:      false,
+		},
+	}
+
+	for _, tc := range tests {
+		fs := afero.NewMemMapFs()
+
+		if tc.appFilePath != "" {
+			stageFile(t, fs, tc.appFilePath, "/app.yaml")
+		}
+
+		ba := newBaseApp(fs, "/")
+
+		// Test updating non-existing registry
+		err := ba.UpdateLib(tc.libCfg.Name, tc.env, &tc.libCfg)
+		if tc.expectErr {
+			require.Error(t, err)
+		} else {
+			require.NoError(t, err)
+		}
+
+		if tc.expectFilePath != "" {
+			assertContents(t, fs, tc.expectFilePath, ba.configPath())
+		}
 	}
 
 }

pkg/app/mocks/App.go

@@ -294,13 +294,13 @@ func (_m *App) SetCurrentEnvironment(name string) error {
 	return r0
 }
 
-// UpdateLib provides a mock function with given fields: name, spec
-func (_m *App) UpdateLib(name string, spec *app.LibraryConfig) error {
-	ret := _m.Called(name, spec)
+// UpdateLib provides a mock function with given fields: name, env, spec
+func (_m *App) UpdateLib(name string, env string, spec *app.LibraryConfig) error {
+	ret := _m.Called(name, env, spec)
 
 	var r0 error
-	if rf, ok := ret.Get(0).(func(string, *app.LibraryConfig) error); ok {
-		r0 = rf(name, spec)
+	if rf, ok := ret.Get(0).(func(string, string, *app.LibraryConfig) error); ok {
+		r0 = rf(name, env, spec)
 	} else {
 		r0 = ret.Error(0)
 	}

pkg/app/testdata/pkg-install-env-scope.yaml

+apiVersion: 0.2.0
+environments:
+  default:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    libraries:
+      nginx:
+        name: nginx
+        registry: incubator
+        version: 1.2.3
+    path: default
+  us-east/test:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    path: us-east/test
+  us-west/prod:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    path: us-west/prod
+  us-west/test:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    path: us-west/test
+kind: ksonnet.io/app
+name: test-get-envs
+registries:
+  incubator:
+    protocol: ""
+    uri: ""
+version: 0.0.1

pkg/app/testdata/pkg-install-global.yaml

+apiVersion: 0.2.0
+environments:
+  default:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    path: default
+  us-east/test:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    path: us-east/test
+  us-west/prod:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    path: us-west/prod
+  us-west/test:
+    destination:
+      namespace: some-namespace
+      server: http://example.com
+    k8sVersion: v1.7.0
+    path: us-west/test
+kind: ksonnet.io/app
+libraries:
+  nginx:
+    name: nginx
+    registry: incubator
+    version: 1.2.3
+name: test-get-envs
+registries:
+  incubator:
+    protocol: ""
+    uri: ""
+version: 0.0.1

pkg/clicmd/pkg_install.go

@@ -27,6 +27,7 @@ import (
 
 var (
 	vPkgInstallName = "pkg-install-name"
+	vPkgInstallEnv  = "pkg-install-env"
 
 	pkgInstallLong = `
 The ` + "`install`" + ` command caches a ksonnet library locally, and makes it available
@@ -46,7 +47,7 @@ channels for official ksonnet libraries.
 ### Syntax
 `
 	pkgInstallExample = `
-# Install an nginx dependency, based on the latest branch.
+# Install an nginx dependency, based on the tip defined by the registry URI.
 # In a ksonnet source file, this can be referenced as:
 #   local nginx = import "incubator/nginx/nginx.libsonnet";
 ks pkg install incubator/nginx
@@ -55,6 +56,11 @@ ks pkg install incubator/nginx
 # In a ksonnet source file, this can be referenced as:
 #   local nginx = import "incubator/nginx/nginx.libsonnet";
 ks pkg install incubator/nginx@master
+
+# Install a specific nginx version into the stage environment.
+# In a ksonnet source file, this can be referenced as:
+#   local nginx = import "incubator/nginx/nginx.libsonnet";
+ks pkg install --env stage incubator/nginx@40285d8a14f1ac5787e405e1023cf0c07f6aa28c
 `
 )
 
@@ -75,6 +81,7 @@ func newPkgInstallCmd(a app.App) *cobra.Command {
 				actions.OptionApp:     a,
 				actions.OptionLibName: args[0],
 				actions.OptionName:    viper.GetString(vPkgInstallName),
+				actions.OptionEnvName: viper.GetString(vPkgInstallEnv),
 			}
 
 			return runAction(actionPkgInstall, m)
@@ -82,7 +89,9 @@ func newPkgInstallCmd(a app.App) *cobra.Command {
 	}
 
 	pkgInstallCmd.Flags().String(flagName, "", "Name to give the dependency, to use within the ksonnet app")
+	pkgInstallCmd.Flags().String(flagEnv, "", "Environment to install package into (optional)")
 	viper.BindPFlag(vPkgInstallName, pkgInstallCmd.Flags().Lookup(flagName))
+	viper.BindPFlag(vPkgInstallEnv, pkgInstallCmd.Flags().Lookup(flagEnv))
 
 	return pkgInstallCmd
 }

pkg/clicmd/pkg_install_test.go

@@ -31,6 +31,18 @@ func Test_pkgInstallCmd(t *testing.T) {
 				actions.OptionApp:     nil,
 				actions.OptionLibName: "package-name",
 				actions.OptionName:    "",
+				actions.OptionEnvName: "",
+			},
+		},
+		{
+			name:   "with env flag",
+			args:   []string{"pkg", "install", "--env", "production", "package-name"},
+			action: actionPkgInstall,
+			expected: map[string]interface{}{
+				actions.OptionApp:     nil,
+				actions.OptionLibName: "package-name",
+				actions.OptionName:    "",
+				actions.OptionEnvName: "production",
 			},
 		},
 		{

pkg/registry/cache.go

@@ -30,7 +30,7 @@ import (
 // CacheDependency vendors registry dependencies.
 // TODO: create unit tests for this once mocks for this package are
 // worked out.
-func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, customName string) error {
+func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, customName string) (*app.LibraryConfig, error) {
 	logger := log.WithFields(log.Fields{
 		"action":      "registry.CacheDependency",
 		"part":        d.Name,
@@ -40,27 +40,27 @@ func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, cust
 	})
 
 	if a == nil {
-		return errors.Errorf("nil receiver")
+		return nil, errors.Errorf("nil receiver")
 	}
 	if checker == nil {
-		return errors.Errorf("nil installation checker")
+		return nil, errors.Errorf("nil installation checker")
 	}
 
 	logger.Debug("caching dependency")
 
 	registries, err := a.Registries()
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	regRefSpec, exists := registries[d.Registry]
 	if !exists {
-		return fmt.Errorf("registry '%s' does not exist", d.Registry)
+		return nil, fmt.Errorf("registry '%s' does not exist", d.Registry)
 	}
 
 	r, err := Locate(a, regRefSpec)
 	if err != nil {
-		return err
+		return nil, err
 	}
 
 	// Get all directories and files first, then write to disk. This
@@ -80,7 +80,7 @@ func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, cust
 			return nil
 		})
 	if err != nil {
-		return errors.Wrap(err, "resolve registry library")
+		return nil, errors.Wrap(err, "resolve registry library")
 	}
 
 	// Make triple-sure the library references the correct registry, as it is known in this app.
@@ -91,10 +91,10 @@ func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, cust
 	qualified.Version = libRef.Version
 	ok, err := checker.IsInstalled(qualified)
 	if err != nil {
-		return errors.Wrapf(err, "checking package installed status")
+		return nil, errors.Wrapf(err, "checking package installed status")
 	}
 	if ok {
-		return errors.Errorf("package '%s/%s@%s' already exists.",
+		return nil, errors.Errorf("package '%s/%s@%s' already exists.",
 			libRef.Registry, libRef.Name, libRef.Version)
 	}
 
@@ -104,7 +104,7 @@ func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, cust
 
 	for _, dir := range directories {
 		if err = a.Fs().MkdirAll(dir, app.DefaultFolderPermissions); err != nil {
-			return errors.Wrap(err, "unable to create directory")
+			return nil, errors.Wrap(err, "unable to create directory")
 		}
 	}
 
@@ -119,15 +119,15 @@ func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, cust
 
 		log.Debugf("onFile: vendoring file to path: %v", vendoredPath)
 		if err = a.Fs().MkdirAll(dir, app.DefaultFolderPermissions); err != nil {
-			return errors.Wrap(err, "unable to create directory")
+			return nil, errors.Wrap(err, "unable to create directory")
 		}
 
 		if err = afero.WriteFile(a.Fs(), vendoredPath, content, app.DefaultFilePermissions); err != nil {
-			return errors.Wrap(err, "unable to create file")
+			return nil, errors.Wrap(err, "unable to create file")
 		}
 	}