Registry cache behavior - do not pin specific version, automatic sliding forward on branches

Additional work:

* Rework ks set update -> ks registry set
* Registries are no longer versioned
* Deprecation of GitVersion in configuration files

Issues:

Rework logic from #604.
Part of #237.
Signed-off-by: Oren Shomron <shomron@gmail.com>

pkg/actions/init.go

@@ -118,7 +118,7 @@ func (i *Init) Run() error {
 func initIncubator(a app.App) (registry.Registry, error) {
 	return registry.NewGitHub(
 		a,
-		&app.RegistryRefSpec{
+		&app.RegistryConfig{
 			Name:     "incubator",
 			Protocol: string(registry.ProtocolGitHub),
 			URI:      defaultIncubatorURI,

pkg/actions/pkg_describe_test.go

@@ -27,8 +27,8 @@ import (
 
 func TestPkgDescribe_library(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{
-			"apache": &app.LibraryRefSpec{},
+		libaries := app.LibraryConfigs{
+			"apache": &app.LibraryConfig{},
 		}
 		appMock.On("Libraries").Return(libaries, nil)
 
@@ -61,8 +61,8 @@ func TestPkgDescribe_library(t *testing.T) {
 
 func TestPkgDescribe_registry(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		registries := app.RegistryRefSpecs{
-			"incubator": &app.RegistryRefSpec{},
+		registries := app.RegistryConfigs{
+			"incubator": &app.RegistryConfig{},
 		}
 		appMock.On("Registries").Return(registries, nil)
 

pkg/actions/pkg_install_test.go

@@ -51,11 +51,11 @@ func TestPkgInstall(t *testing.T) {
 
 		a.depCacherFn = dc
 
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 		appMock.On("Libraries").Return(libaries, nil)
 
-		registries := app.RegistryRefSpecs{
-			"incubator": &app.RegistryRefSpec{
+		registries := app.RegistryConfigs{
+			"incubator": &app.RegistryConfig{
 				Protocol: string(registry.ProtocolFilesystem),
 				URI:      "file:///tmp",
 			},

pkg/actions/pkg_list_test.go

@@ -27,16 +27,16 @@ import (
 
 func TestPkgList(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{
-			"lib1": &app.LibraryRefSpec{},
+		libaries := app.LibraryConfigs{
+			"lib1": &app.LibraryConfig{},
 		}
 
 		appMock.On("Libraries").Return(libaries, nil)
 
 		spec := &registry.Spec{
-			Libraries: registry.LibraryRefSpecs{
-				"lib1": &registry.LibraryRef{},
-				"lib2": &registry.LibraryRef{},
+			Libraries: registry.LibraryConfigs{
+				"lib1": &registry.LibaryConfig{},
+				"lib2": &registry.LibaryConfig{},
 			},
 		}
 

pkg/actions/prototype_describe_test.go

@@ -26,7 +26,7 @@ import (
 
 func TestPrototypeDescribe(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 

pkg/actions/prototype_list_test.go

@@ -26,7 +26,7 @@ import (
 
 func TestPrototypeList(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 

pkg/actions/prototype_preview_test.go

@@ -29,7 +29,7 @@ import (
 
 func TestPrototypePreview(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 
@@ -60,7 +60,7 @@ func TestPrototypePreview(t *testing.T) {
 
 func TestPrototypePreview_bind_flags_failed(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 

pkg/actions/prototype_search_test.go

@@ -27,7 +27,7 @@ import (
 
 func TestPrototypeSearch(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 

pkg/actions/prototype_use_test.go

@@ -30,7 +30,7 @@ import (
 
 func TestPrototypeUse(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 
@@ -75,7 +75,7 @@ func TestPrototypeUse(t *testing.T) {
 
 func TestPrototypeUse_bind_flags_failed(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 
@@ -123,7 +123,7 @@ func TestPrototypeUse_bind_flags_failed(t *testing.T) {
 
 func TestPrototypeUse_with_module_in_name(t *testing.T) {
 	withApp(t, func(appMock *amocks.App) {
-		libaries := app.LibraryRefSpecs{}
+		libaries := app.LibraryConfigs{}
 
 		appMock.On("Libraries").Return(libaries, nil)
 

pkg/actions/registry_describe.go

@@ -41,7 +41,7 @@ type RegistryDescribe struct {
 	app                 app.App
 	name                string
 	out                 io.Writer
-	fetchRegistrySpecFn func(a app.App, name string) (*registry.Spec, *app.RegistryRefSpec, error)
+	fetchRegistrySpecFn func(a app.App, name string) (*registry.Spec, *app.RegistryConfig, error)
 }
 
 // NewRegistryDescribe creates an instance of RegistryDescribe
@@ -93,7 +93,7 @@ func (rd *RegistryDescribe) Run() error {
 	return nil
 }
 
-func fetchRegistrySpec(a app.App, name string) (*registry.Spec, *app.RegistryRefSpec, error) {
+func fetchRegistrySpec(a app.App, name string) (*registry.Spec, *app.RegistryConfig, error) {
 	appRegistries, err := a.Registries()
 	if err != nil {
 		return nil, nil, err

pkg/actions/registry_describe_test.go

@@ -38,26 +38,26 @@ func TestRegistryDescribe(t *testing.T) {
 		var buf bytes.Buffer
 		a.out = &buf
 
-		a.fetchRegistrySpecFn = func(a app.App, name string) (*registry.Spec, *app.RegistryRefSpec, error) {
+		a.fetchRegistrySpecFn = func(a app.App, name string) (*registry.Spec, *app.RegistryConfig, error) {
 			require.Equal(t, "incubator", name)
 
 			spec := &registry.Spec{
-				Libraries: registry.LibraryRefSpecs{
-					"apache":    &registry.LibraryRef{Path: "apache"},
-					"efk":       &registry.LibraryRef{Path: "efk"},
-					"mariadb":   &registry.LibraryRef{Path: "mariadb"},
-					"memcached": &registry.LibraryRef{Path: "memcached"},
-					"mongodb":   &registry.LibraryRef{Path: "mongodb"},
-					"mysql":     &registry.LibraryRef{Path: "mysql"},
-					"nginx":     &registry.LibraryRef{Path: "nginx"},
-					"node":      &registry.LibraryRef{Path: "node"},
-					"postres":   &registry.LibraryRef{Path: "postgres"},
-					"redis":     &registry.LibraryRef{Path: "redis"},
-					"tomcat":    &registry.LibraryRef{Path: "tomcat"},
+				Libraries: registry.LibraryConfigs{
+					"apache":    &registry.LibaryConfig{Path: "apache"},
+					"efk":       &registry.LibaryConfig{Path: "efk"},
+					"mariadb":   &registry.LibaryConfig{Path: "mariadb"},
+					"memcached": &registry.LibaryConfig{Path: "memcached"},
+					"mongodb":   &registry.LibaryConfig{Path: "mongodb"},
+					"mysql":     &registry.LibaryConfig{Path: "mysql"},
+					"nginx":     &registry.LibaryConfig{Path: "nginx"},
+					"node":      &registry.LibaryConfig{Path: "node"},
+					"postres":   &registry.LibaryConfig{Path: "postgres"},
+					"redis":     &registry.LibaryConfig{Path: "redis"},
+					"tomcat":    &registry.LibaryConfig{Path: "tomcat"},
 				},
 			}
 
-			regRef := &app.RegistryRefSpec{
+			regRef := &app.RegistryConfig{
 				Name:     "incubator",
 				URI:      "github.com/ksonnet/parts/tree/master/incubator",
 				Protocol: "github",

pkg/actions/registry_set.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.
+
+package actions
+
+import (
+	"github.com/ksonnet/ksonnet/pkg/app"
+	"github.com/ksonnet/ksonnet/pkg/registry"
+	"github.com/pkg/errors"
+	log "github.com/sirupsen/logrus"
+)
+
+// RunRegistrySet runs `env list`
+func RunRegistrySet(m map[string]interface{}) error {
+	ru, err := NewRegistrySet(m)
+	if err != nil {
+		return err
+	}
+
+	ol := newOptionLoader(m)
+	name := ol.LoadString(OptionName)
+	uri := ol.LoadString(OptionURI)
+
+	if ol.err != nil {
+		return ol.err
+	}
+
+	return ru.run(name, uri)
+}
+
+type locateFn func(app.App, *app.RegistryConfig) (registry.Setter, error)
+
+// RegistrySet lists available registries
+type RegistrySet struct {
+	app      app.App
+	locateFn locateFn
+}
+
+// NewRegistrySet creates an instance of RegistrySet
+func NewRegistrySet(m map[string]interface{}) (*RegistrySet, error) {
+	ol := newOptionLoader(m)
+
+	rs := &RegistrySet{
+		app:      ol.LoadApp(),
+		locateFn: defaultLocate,
+	}
+
+	if ol.err != nil {
+		return nil, ol.err
+	}
+
+	return rs, nil
+}
+
+// defaultLocate passes-through to registry.Locate, but constrains the interface
+// to just `registry.Setter`. The concrete type of registry.Setter is determined
+// by the `spec` argument. In other words, this is a factory for registry.Setter implementations.
+func defaultLocate(ksApp app.App, spec *app.RegistryConfig) (registry.Setter, error) {
+	return registry.Locate(ksApp, spec)
+}
+
+// registryConfig returns a registry configuration by name from the provided App.
+func registryConfig(a app.App, name string) (*app.RegistryConfig, error) {
+	if name == "" {
+		return nil, errors.Errorf("registry name required")
+	}
+
+	if a == nil {
+		return nil, errors.Errorf("missing application")
+	}
+
+	// NOTE: app.Registries() does not currently cache app configuration
+	specs, err := a.Registries()
+	if err != nil {
+		return nil, errors.Wrap(err, "retrieving configured registries")
+	}
+
+	cfg, ok := specs[name]
+	if !ok {
+		return nil, errors.Errorf("unknown registry: %v", name)
+	}
+	return cfg, nil
+}
+
+// run runs the registry set command.
+func (rs *RegistrySet) run(name string, uri string) error {
+	if rs == nil {
+		return errors.Errorf("nil receiver")
+	}
+
+	cfg, err := registryConfig(rs.app, name)
+	if err != nil {
+		return err
+	}
+
+	return doSetURI(rs.app, rs.locateFn, cfg, uri)
+}
+
+// doSetURI sets the URI for the specified registry.
+func doSetURI(a app.App, locateFn locateFn, cfg *app.RegistryConfig, uri string) error {
+	if a == nil {
+		return errors.Errorf("missing application")
+	}
+
+	if locateFn == nil {
+		return errors.Errorf("missing registry locator function")
+	}
+
+	if uri == "" {
+		return errors.Errorf("nothing to set")
+	}
+
+	// Lookup a registry.Setter implementation
+	setter, err := locateFn(a, cfg)
+	if err != nil || setter == nil {
+		return errors.Wrap(err, "retrieving registry setter")
+	}
+
+	// Capture current settings (make a copy!!)
+	var oldCfg app.RegistryConfig
+	oldCfg = *setter.MakeRegistryConfig()
+
+	log.Debugf("setting registry %v uri: %v", cfg.Name, uri)
+	if err := setter.SetURI(uri); err != nil {
+		return errors.Wrapf(err, "setting registry %v uri: %v", cfg.Name, uri)
+	}
+
+	// Update app only if a change has occurred
+	newCfg := setter.MakeRegistryConfig()
+	if oldCfg.URI == newCfg.URI {
+		log.Debugf("registry %v unchanged", cfg.Name)
+		return nil
+	}
+
+	// Persist changes back to app.yaml
+	if err := a.UpdateRegistry(newCfg); err != nil {
+		return errors.Wrapf(err, "updating registry %v in app", cfg.Name)
+	}
+
+	// Update registry cache
+	if _, err := setter.FetchRegistrySpec(); err != nil {
+		return errors.Wrap(err, "cache registry")
+	}
+
+	return nil
+}

pkg/actions/registry_update_test.go → pkg/actions/registry_set_test.go

@@ -16,7 +16,6 @@
 package actions
 
 import (
-	"sort"
 	"testing"
 
 	"github.com/ksonnet/ksonnet/pkg/app"
@@ -30,232 +29,188 @@ import (
 
 func TestRegistryUpdate_requires_app(t *testing.T) {
 	in := make(map[string]interface{})
-	_, err := NewRegistryUpdate(in)
+	_, err := NewRegistrySet(in)
 	require.Error(t, err)
 }
 
-// Generate spec->registry.Updater locators with customized mock registries
-func mockRegistryLocator(oldVersion string, newVersion string) LocateFn {
-	return func(app.App, *app.RegistryRefSpec) (registry.Updater, error) {
-		u := new(rmocks.Updater)
-		u.On("Update", oldVersion).Return(func(v string) (string, error) {
-			return newVersion, nil
-		})
-		return u, nil
+// Generate spec->registry.Setter locators with customized mock registries
+func mockLocator(name string, oldURI string, newURI string) locateFn {
+	return func(app.App, *app.RegistryConfig) (registry.Setter, error) {
+		return mockSetter(name, oldURI, newURI), nil
 	}
 }
 
-func mockUpdater(oldVersion string, newVersion string) registry.Updater {
-	u := new(rmocks.Updater)
-	u.On("Update", oldVersion).Return(newVersion, nil)
+func mockSetter(name string, oldURI string, newURI string) registry.Setter {
+	i := 0
+	returnValues := []*app.RegistryConfig{
+		&app.RegistryConfig{
+			Name: name,
+			URI:  oldURI,
+		},
+		&app.RegistryConfig{
+			Name: name,
+			URI:  newURI,
+		},
+	}
+
+	u := new(rmocks.Setter)
+	u.On("SetURI", newURI).Return(nil)
+	u.On("MakeRegistryConfig").Return(
+		func() *app.RegistryConfig {
+			if i > len(returnValues)-1 {
+				return nil
+			}
+
+			ret := returnValues[i]
+			i++
+			return ret
+		},
+	)
+	u.On("FetchRegistrySpec").Return(nil, nil)
 	return u
 }
 
-// Test that a set of registries to update can be resolved, one specific or all if unspecified.
-func TestRegistryUpdate_resolveUpdateSet(t *testing.T) {
+// Test lookup of registry configuration by name
+func TestRegistrySet_registryConfig(t *testing.T) {
 	a := new(amocks.App)
-	a.On("Registries").Return(
-		app.RegistryRefSpecs{
-			"custom":    nil,
-			"helm":      nil,
-			"incubator": nil,
+	registries := app.RegistryConfigs{
+		"incubator": &app.RegistryConfig{
+			Name:     "incubator",
+			Protocol: string(registry.ProtocolGitHub),
+			URI:      "github.com/ksonnet/parts/tree/master/incubator",
 		},
-		nil,
-	)
-	ru := &RegistryUpdate{
-		app: a,
 	}
+	a.On("Registries").Return(registries, nil)
 
 	tests := []struct {
 		caseName  string
 		name      string
-		expected  []string
+		app       app.App
+		expected  *app.RegistryConfig
 		expectErr bool
 	}{
-		{
-			caseName:  "all registries",
-			name:      "",
-			expected:  []string{"custom", "helm", "incubator"},
-			expectErr: false,
-		},
 		{
 			caseName:  "specific registry",
 			name:      "incubator",
-			expected:  []string{"incubator"},
+			app:       a,
+			expected:  registries["incubator"],
 			expectErr: false,
 		},
 		{
 			caseName:  "unknown registry",
 			name:      "unknown",
-			expected:  []string{},
+			app:       a,
+			expected:  nil,
+			expectErr: true,
+		},
+		{
+			caseName:  "empty name",
+			name:      "",
+			app:       a,
+			expected:  nil,
+			expectErr: true,
+		},
+		{
+			caseName:  "nil app",
+			name:      "unknown",
+			app:       nil,
+			expected:  nil,
 			expectErr: true,
 		},
 	}
 
 	for _, tc := range tests {
-		result, err := ru.resolveUpdateSet(tc.name)
+		result, err := registryConfig(tc.app, tc.name)
 		if tc.expectErr {
 			require.Errorf(t, err, "test: %v", tc.name)
 		} else {
 			require.NoErrorf(t, err, "test: %v", tc.name)
 		}
 
-		// Don't make assertions about return value if error was returned
-		if err != nil {
-			continue
-		}
-
-		sort.Strings(result)
 		assert.Equal(t, tc.expected, result)
 	}
 }
 
-func TestRegistryUpdate_doUpdateRegistry(t *testing.T) {
+func TestRegistryUpdate_doSetURI(t *testing.T) {
 	// Helpers
-	makeApp := func(newVersion string) *amocks.App {
+	makeApp := func(expectedName string, expectedURI string) *amocks.App {
 		a := new(amocks.App)
 		a.On("UpdateRegistry",
-			mock.MatchedBy(func(spec *app.RegistryRefSpec) bool {
-				if spec == nil {
-					t.Errorf("spec is nil")
+			mock.MatchedBy(func(spec *app.RegistryConfig) bool {
+				if !assert.NotNil(t, spec) {
 					return false
 				}
-				if spec.GitVersion == nil {
-					t.Errorf("spec.GitVersion is nil")
+				if !assert.Equal(t, expectedName, spec.Name) {
 					return false
 				}
-				if spec.GitVersion.CommitSHA != newVersion {
-					t.Errorf("unexpected version argument: expected %v, got %v", newVersion, spec.GitVersion.CommitSHA)
+
+				if !assert.Equal(t, expectedURI, spec.URI) {
 					return false
 				}
+
 				return true
 			}),
 		).Return(nil).Once()
 		return a
 	}
 
-	makeSpec := func(version string) *app.RegistryRefSpec {
-		return &app.RegistryRefSpec{
-			GitVersion: &app.GitVersionSpec{
-				CommitSHA: version,
-			},
+	makeSpec := func(name string, uri string) *app.RegistryConfig {
+		return &app.RegistryConfig{
+			Name: name,
+			URI:  uri,
 		}
 	}
 
 	tests := []struct {
-		name             string
-		app              *amocks.App
-		updater          registry.Updater
-		rs               *app.RegistryRefSpec
-		requestedVersion string
-		expected         string
-		shouldUpdate     bool
-		expectErr        bool
+		name         string
+		oldURI       string
+		newURI       string
+		shouldUpdate bool
+		expectErr    bool
 	}{
 		{
-			name:             "normal update",
-			app:              makeApp("newVersion"),
-			updater:          mockUpdater("", "newVersion"),
-			rs:               makeSpec("currentVersion"),
-			requestedVersion: "",
-			expected:         "newVersion",
-			shouldUpdate:     true,
-			expectErr:        false,
-		},
-		{
-			name:             "no change, shouldn't update",
-			app:              makeApp("XXXX"),
-			updater:          mockUpdater("", "currentVersion"),
-			rs:               makeSpec("currentVersion"),
-			requestedVersion: "",
-			expected:         "currentVersion",
-			shouldUpdate:     false,