pkg install honors versions when checking for conflicts

This commit allows installing a package when a different version is
already installed. Currently, if the package is installed globally (the
only option) - it will overwrite the previous version reference.
A followup change will allow installing into environments to work around
this limitation.

Part of #660

Also:

* Add PackageManager.IsInstalled - fuzzy-match installation check for packages, without parsing their manifests
Signed-off-by: Oren Shomron <shomron@gmail.com>

pkg/actions/pkg_install.go

@@ -22,7 +22,7 @@ import (
 )
 
 // DepCacher is a function that caches a dependency.j
-type DepCacher func(app.App, pkg.Descriptor, string) error
+type DepCacher func(app.App, registry.InstalledChecker, pkg.Descriptor, string) error
 
 // RunPkgInstall runs `pkg install`
 func RunPkgInstall(m map[string]interface{}) error {
@@ -39,6 +39,7 @@ type PkgInstall struct {
 	app         app.App
 	libName     string
 	customName  string
+	checker     registry.InstalledChecker
 	depCacherFn DepCacher
 }
 
@@ -46,10 +47,16 @@ type PkgInstall struct {
 func NewPkgInstall(m map[string]interface{}) (*PkgInstall, error) {
 	ol := newOptionLoader(m)
 
+	a := ol.LoadApp()
+	if ol.err != nil {
+		return nil, ol.err
+	}
+
 	nl := &PkgInstall{
-		app:        ol.LoadApp(),
+		app:        a,
 		libName:    ol.LoadString(OptionLibName),
 		customName: ol.LoadString(OptionName),
+		checker:    registry.NewPackageManager(a),
 
 		depCacherFn: registry.CacheDependency,
 	}
@@ -68,7 +75,7 @@ func (pi *PkgInstall) Run() error {
 		return err
 	}
 
-	return pi.depCacherFn(pi.app, d, customName)
+	return pi.depCacherFn(pi.app, pi.checker, d, customName)
 }
 
 func (pi *PkgInstall) parseDepSpec() (pkg.Descriptor, string, error) {

pkg/actions/pkg_install_test.go

@@ -30,7 +30,7 @@ func TestPkgInstall(t *testing.T) {
 		libName := "incubator/apache"
 		customName := "customName"
 
-		dc := func(a app.App, d pkg.Descriptor, cn string) error {
+		dc := func(a app.App, checker registry.InstalledChecker, d pkg.Descriptor, cn string) error {
 			expectedD := pkg.Descriptor{
 				Registry: "incubator",
 				Name:     "apache",

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, d pkg.Descriptor, customName string) error {
+func CacheDependency(a app.App, checker InstalledChecker, d pkg.Descriptor, customName string) error {
 	logger := log.WithFields(log.Fields{
 		"action":      "registry.CacheDependency",
 		"part":        d.Name,
@@ -39,18 +39,15 @@ func CacheDependency(a app.App, d pkg.Descriptor, customName string) error {
 		"custom-name": customName,
 	})
 
-	logger.Debug("caching dependency")
-
-	libs, err := a.Libraries()
-	if err != nil {
-		return err
+	if a == nil {
+		return errors.Errorf("nil receiver")
 	}
-
-	if _, ok := libs[customName]; ok {
-		return errors.Errorf("package '%s' already exists. Use the --name flag to install this package with a unique identifier",
-			customName)
+	if checker == nil {
+		return errors.Errorf("nil installation checker")
 	}
 
+	logger.Debug("caching dependency")
+
 	registries, err := a.Registries()
 	if err != nil {
 		return err
@@ -89,6 +86,18 @@ func CacheDependency(a app.App, d pkg.Descriptor, customName string) error {
 	// Make triple-sure the library references the correct registry, as it is known in this app.
 	libRef.Registry = d.Registry
 
+	// Check whether this library version is already installed
+	var qualified = d
+	qualified.Version = libRef.Version
+	ok, err := checker.IsInstalled(qualified)
+	if err != nil {
+		return errors.Wrapf(err, "checking package installed status")
+	}
+	if ok {
+		return errors.Errorf("package '%s/%s@%s' already exists.",
+			libRef.Registry, libRef.Name, libRef.Version)
+	}
+
 	// Add library to app specification, but wait to write it out until
 	// the end, in case one of the network calls fails.
 	log.Infof("Retrieved %d files", len(files))

pkg/registry/cache_test.go

@@ -28,6 +28,14 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+// We can't currently import registry/mocks due to a cycle.
+// Implement simple mock registry.InstalledChecker.
+type installedChecker struct{}
+
+func (_m *installedChecker) IsInstalled(d pkg.Descriptor) (bool, error) {
+	return false, nil
+}
+
 func Test_CacheDependency(t *testing.T) {
 	withApp(t, func(a *amocks.App, fs afero.Fs) {
 		a.On("VendorPath").Return("/app/vendor")
@@ -59,9 +67,11 @@ func Test_CacheDependency(t *testing.T) {
 		}
 		for _, lib := range libs {
 			a.On("UpdateLib", lib.Name, lib).Return(nil)
+
+			var checker installedChecker
 			d := pkg.Descriptor{Registry: lib.Registry, Name: lib.Name}
 
-			err := CacheDependency(a, d, "")
+			err := CacheDependency(a, &checker, d, "")
 			require.NoError(t, err)
 
 			test.AssertExists(t, fs, filepath.Join(a.Root(), "vendor", lib.Registry, lib.Name, "parts.yaml"))

pkg/registry/mocks/InstalledChecker.go

+// Copyright 2018 The ksonnet authors
+//
+//
+//    Licensed under the Apache License, Version 2.0 (the "License");
+//    you may not use this file except in compliance with the License.
+//    You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+//    Unless required by applicable law or agreed to in writing, software
+//    distributed under the License is distributed on an "AS IS" BASIS,
+//    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//    See the License for the specific language governing permissions and
+//    limitations under the License.
+
+// Code generated by mockery v1.0.0. DO NOT EDIT.
+package mocks
+
+import mock "github.com/stretchr/testify/mock"
+import pkg "github.com/ksonnet/ksonnet/pkg/pkg"
+
+// InstalledChecker is an autogenerated mock type for the InstalledChecker type
+type InstalledChecker struct {
+	mock.Mock
+}
+
+// IsInstalled provides a mock function with given fields: d
+func (_m *InstalledChecker) IsInstalled(d pkg.Descriptor) (bool, error) {
+	ret := _m.Called(d)
+
+	var r0 bool
+	if rf, ok := ret.Get(0).(func(pkg.Descriptor) bool); ok {
+		r0 = rf(d)
+	} else {
+		r0 = ret.Get(0).(bool)
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(pkg.Descriptor) error); ok {
+		r1 = rf(d)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}

pkg/registry/mocks/PackageManager.go

@@ -49,6 +49,27 @@ func (_m *PackageManager) Find(_a0 string) (pkg.Package, error) {
 	return r0, r1
 }
 
+// IsInstalled provides a mock function with given fields: d
+func (_m *PackageManager) IsInstalled(d pkg.Descriptor) (bool, error) {
+	ret := _m.Called(d)
+
+	var r0 bool
+	if rf, ok := ret.Get(0).(func(pkg.Descriptor) bool); ok {
+		r0 = rf(d)
+	} else {
+		r0 = ret.Get(0).(bool)
+	}
+
+	var r1 error
+	if rf, ok := ret.Get(1).(func(pkg.Descriptor) error); ok {
+		r1 = rf(d)
+	} else {
+		r1 = ret.Error(1)
+	}
+
+	return r0, r1
+}
+
 // Packages provides a mock function with given fields:
 func (_m *PackageManager) Packages() ([]pkg.Package, error) {
 	ret := _m.Called()

pkg/registry/mocks/generate.go

@@ -18,3 +18,4 @@ package mocks
 //go:generate mockery -dir .. -output . -name=Registry
 //go:generate mockery -dir .. -output . -name=Setter
 //go:generate mockery -dir .. -output . -name=PackageManager
+//go:generate mockery -dir .. -output . -name=InstalledChecker

pkg/registry/package_manager.go

@@ -25,6 +25,13 @@ import (
 	"github.com/pkg/errors"
 )
 
+// InstalledChecker checks if a package is installed, based on app config.
+type InstalledChecker interface {
+	// IsInstalled checks whether a package is installed.
+	// Supports fuzzy searches: Name, Name/Version, Registry/Name/Version, Registry/Version
+	IsInstalled(d pkg.Descriptor) (bool, error)
+}
+
 // PackageManager is a package manager.
 type PackageManager interface {
 	Find(string) (pkg.Package, error)
@@ -38,6 +45,8 @@ type PackageManager interface {
 
 	// Prototypes lists prototypes.
 	Prototypes() (prototype.Prototypes, error)
+
+	InstalledChecker
 }
 
 // packageManager is an implementation of PackageManager.
@@ -58,6 +67,7 @@ func NewPackageManager(a app.App) PackageManager {
 }
 
 // Find finds a package by name. Package names have the format `<registry>/<library>@<version>`.
+// Remote registries may be consulted if the package is not installed locally.
 func (m *packageManager) Find(name string) (pkg.Package, error) {
 	d, err := pkg.Parse(name)
 	if err != nil {
@@ -298,3 +308,93 @@ func (m *packageManager) Prototypes() (prototype.Prototypes, error) {
 
 	return prototypes, nil
 }
+
+type libraryByVersion map[string]*app.LibraryConfig
+
+// Index libraries by descriptor, each can have multiple distinct versions.
+// The same library can be indexed under multiple permutations of its fully-qualified descriptor
+type libraryByDesc map[pkg.Descriptor]libraryByVersion
+
+func indexLibrary(index libraryByDesc, d pkg.Descriptor, l *app.LibraryConfig) {
+	byVer, ok := index[d]
+	if !ok {
+		byVer = libraryByVersion{}
+		index[d] = byVer
+	}
+
+	byVer[d.Version] = l
+}
+
+func indexLibraryPermutations(index libraryByDesc, l *app.LibraryConfig) {
+	if l == nil {
+		return
+	}
+
+	d := pkg.Descriptor{Name: l.Name, Registry: l.Registry, Version: l.Version}
+	indexLibrary(index, d, l)
+
+	d = pkg.Descriptor{Name: l.Name, Registry: "", Version: l.Version}
+	indexLibrary(index, d, l)
+
+	d = pkg.Descriptor{Name: l.Name, Registry: l.Registry, Version: ""}
+	indexLibrary(index, d, l)
+
+	d = pkg.Descriptor{Name: l.Name, Registry: "", Version: ""}
+	indexLibrary(index, d, l)
+}
+
+// Returns index of library configurations for the app.
+// Libraries are indexed using multiple permutations to aid
+// in search using partial keys.
+func allLibraries(a app.App) (libraryByDesc, error) {
+	if a == nil {
+		return nil, errors.Errorf("nil receiver")
+	}
+
+	index := libraryByDesc{}
+
+	libs, err := a.Libraries()
+	if err != nil {
+		return nil, errors.Wrapf(err, "checking libraries")
+	}
+	for _, l := range libs {
+		indexLibraryPermutations(index, l)
+	}
+
+	envs, err := a.Environments()
+	if err != nil {
+		return nil, errors.Wrapf(err, "checking environments")
+	}
+	for _, env := range envs {
+		for _, l := range env.Libraries {
+			indexLibraryPermutations(index, l)
+		}
+	}
+
+	return index, nil
+}
+
+// IsInstalled determines whether the specified package is installed.
+// Only Name is required in the descriptor, Registry and Version are inferred as "any"
+// if missing. IsInstalled will make as specific a match as possible.
+func (m *packageManager) IsInstalled(d pkg.Descriptor) (bool, error) {
+	if m == nil {
+		return false, errors.Errorf("nil receiver")
+	}
+	a := m.app
+	if a == nil {
+		return false, errors.Errorf("nil app")
+	}
+
+	if d.Name == "" {
+		return false, errors.Errorf("name required")
+	}
+
+	index, err := allLibraries(a)
+	if err != nil {
+		return false, errors.Wrapf(err, "indexing libraries")
+	}
+
+	byVer := index[d]
+	return len(byVer) > 0, nil
+}

pkg/registry/package_manager_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/ksonnet/ksonnet/pkg/app"
 	amocks "github.com/ksonnet/ksonnet/pkg/app/mocks"
 	"github.com/ksonnet/ksonnet/pkg/parts"
+	"github.com/ksonnet/ksonnet/pkg/pkg"
 	"github.com/ksonnet/ksonnet/pkg/util/test"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
@@ -151,3 +152,131 @@ func Test_remotePackage(t *testing.T) {
 	assert.False(t, i)
 	assert.Empty(t, protos)
 }
+
+func Test_IsInstalled(t *testing.T) {
+	libs := app.LibraryConfigs{
+		"mysql": &app.LibraryConfig{
+			Name:     "mysql",
+			Registry: "incubator",
+			Version:  "1.2.3",
+		},
+		"consul": &app.LibraryConfig{
+			Name:     "consul",
+			Registry: "incubator",
+			Version:  "0.6.4",
+		},
+		"unversioned": &app.LibraryConfig{
+			Name:     "unversioned",
+			Registry: "helm",
+			Version:  "",
+		},
+	}
+
+	envs := app.EnvironmentConfigs{
+		"default": &app.EnvironmentConfig{
+			Name: "default",
+			Libraries: app.LibraryConfigs{
+				"mysql": &app.LibraryConfig{
+					Name:     "mysql",
+					Registry: "incubator",
+					Version:  "4.5.6",
+				},
+				"nginx": &app.LibraryConfig{
+					Name:     "nginx",
+					Registry: "incubator",
+					Version:  "0.0.1",
+				},
+			},
+		},
+	}
+
+	tests := []struct {
+		name      string
+		libraries app.LibraryConfigs
+		envs      app.EnvironmentConfigs
+		desc      pkg.Descriptor
+		expected  bool
+	}{
+		{
+			name:      "fully qualified",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "mysql", Registry: "incubator", Version: "1.2.3"},
+			expected:  true,
+		},
+		{
+			name:      "registry/name, any version",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "mysql", Registry: "incubator", Version: ""},
+			expected:  true,
+		},
+		{
+			name:      "just name, any registry, any version",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "mysql", Registry: "", Version: ""},
+			expected:  true,
+		},
+		{
+			name:      "wrong version, qualified registry",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "mysql", Registry: "incubator", Version: "9.9.9"},
+			expected:  false,
+		},
+		{
+			name:      "wrong version, any registry",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "mysql", Registry: "", Version: "9.9.9"},
+			expected:  false,
+		},
+		{
+			name:      "only in environment",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "nginx", Registry: "incubator", Version: ""},
+			expected:  true,
+		},
+		{
+			name:      "only in globals",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "consul", Registry: "incubator", Version: ""},
+			expected:  true,
+		},
+		{
+			name:      "wrong name",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "fake", Registry: "", Version: ""},
+			expected:  false,
+		},
+		{
+			name:      "unversioned library",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "unversioned", Registry: "", Version: ""},
+			expected:  true,
+		},
+		{
+			name:      "unversioned library, versioned search",
+			libraries: libs,
+			envs:      envs,
+			desc:      pkg.Descriptor{Name: "unversioned", Registry: "", Version: "uh-oh"},
+			expected:  false,
+		},
+	}
+
+	for _, tc := range tests {
+		test.WithApp(t, "/app", func(a *amocks.App, fs afero.Fs) {
+			a.On("Libraries").Return(tc.libraries, nil)
+			a.On("Environments").Return(tc.envs, nil)
+			pm := NewPackageManager(a)
+			actual, err := pm.IsInstalled(tc.desc)
+			require.NoError(t, err)
+			assert.Equal(t, tc.expected, actual, tc.name)
+		})
+	}
+}