From 37510aff3ec3d69ca959e5daa4239ffe06f84634 Mon Sep 17 00:00:00 2001 From: fibonacci1729 Date: Tue, 23 Aug 2016 12:14:09 -0600 Subject: [PATCH] chore(pkg/storage): introduce key name convention for storage objects with updated tests --- pkg/storage/driver/cfgmaps.go | 16 +++---- pkg/storage/driver/cfgmaps_test.go | 37 ++++++++------- pkg/storage/driver/driver.go | 4 +- pkg/storage/driver/memory.go | 12 ++--- pkg/storage/driver/memory_test.go | 72 +++++++++++++++++++++++------- pkg/storage/storage.go | 26 +++++++---- pkg/storage/storage_test.go | 14 +++--- 7 files changed, 117 insertions(+), 64 deletions(-) diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index afe031bf5..942634674 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -114,7 +114,7 @@ func (cfgmaps *ConfigMaps) List(filter func(*rspb.Release) bool) ([]*rspb.Releas // Create creates a new ConfigMap holding the release. If the // ConfigMap already exists, ErrReleaseExists is returned. -func (cfgmaps *ConfigMaps) Create(rls *rspb.Release) error { +func (cfgmaps *ConfigMaps) Create(key string, rls *rspb.Release) error { // set labels for configmaps object meta data var lbs labels @@ -122,9 +122,9 @@ func (cfgmaps *ConfigMaps) Create(rls *rspb.Release) error { lbs.set("CREATED_AT", strconv.Itoa(int(time.Now().Unix()))) // create a new configmap to hold the release - obj, err := newConfigMapsObject(rls, lbs) + obj, err := newConfigMapsObject(key, rls, lbs) if err != nil { - logerrf(err, "create: failed to encode release %q", rls.Name) + logerrf(err, "create: failed to encode release %q", key) return err } // push the configmap object out into the kubiverse @@ -141,7 +141,7 @@ func (cfgmaps *ConfigMaps) Create(rls *rspb.Release) error { // Update updates the ConfigMap holding the release. If not found // the ConfigMap is created to hold the release. -func (cfgmaps *ConfigMaps) Update(rls *rspb.Release) error { +func (cfgmaps *ConfigMaps) Update(key string, rls *rspb.Release) error { // set labels for configmaps object meta data var lbs labels @@ -149,9 +149,9 @@ func (cfgmaps *ConfigMaps) Update(rls *rspb.Release) error { lbs.set("MODIFIED_AT", strconv.Itoa(int(time.Now().Unix()))) // create a new configmap object to hold the release - obj, err := newConfigMapsObject(rls, lbs) + obj, err := newConfigMapsObject(key, rls, lbs) if err != nil { - logerrf(err, "update: failed to encode release %q", rls.Name) + logerrf(err, "update: failed to encode release %q", key) return err } // push the configmap object out into the kubiverse @@ -194,7 +194,7 @@ func (cfgmaps *ConfigMaps) Delete(key string) (rls *rspb.Release, err error) { // "OWNER" - owner of the configmap, currently "TILLER". // "NAME" - name of the release. // -func newConfigMapsObject(rls *rspb.Release, lbs labels) (*api.ConfigMap, error) { +func newConfigMapsObject(objkey string, rls *rspb.Release, lbs labels) (*api.ConfigMap, error) { const owner = "TILLER" // encode the release @@ -216,7 +216,7 @@ func newConfigMapsObject(rls *rspb.Release, lbs labels) (*api.ConfigMap, error) // create and return configmap object return &api.ConfigMap{ ObjectMeta: api.ObjectMeta{ - Name: rls.Name, + Name: objkey, Labels: lbs.toMap(), }, Data: map[string]string{"release": s}, diff --git a/pkg/storage/driver/cfgmaps_test.go b/pkg/storage/driver/cfgmaps_test.go index bf2c3f7da..5f473809e 100644 --- a/pkg/storage/driver/cfgmaps_test.go +++ b/pkg/storage/driver/cfgmaps_test.go @@ -37,8 +37,10 @@ func TestConfigMapName(t *testing.T) { } func TestConfigMapGet(t *testing.T) { - key := "key-1" - rel := newTestRelease(key, 1, rspb.Status_DEPLOYED) + vers := int32(1) + name := "smug-pigeon" + key := testKey(name, vers) + rel := newTestRelease(name, vers, rspb.Status_DEPLOYED) cfgmaps := newTestFixture(t, []*rspb.Release{rel}...) @@ -103,11 +105,13 @@ func TestConfigMapList(t *testing.T) { func TestConfigMapCreate(t *testing.T) { cfgmaps := newTestFixture(t) - key := "key-1" - rel := newTestRelease(key, 1, rspb.Status_DEPLOYED) + vers := int32(1) + name := "smug-pigeon" + key := testKey(name, vers) + rel := newTestRelease(name, vers, rspb.Status_DEPLOYED) // store the release in a configmap - if err := cfgmaps.Create(rel); err != nil { + if err := cfgmaps.Create(key, rel); err != nil { t.Fatalf("Failed to create release with key %q: %s", key, err) } @@ -124,16 +128,18 @@ func TestConfigMapCreate(t *testing.T) { } func TestConfigMapUpdate(t *testing.T) { - key := "key-1" - rel := newTestRelease(key, 1, rspb.Status_DEPLOYED) + vers := int32(1) + name := "smug-pigeon" + key := testKey(name, vers) + rel := newTestRelease(name, vers, rspb.Status_DEPLOYED) cfgmaps := newTestFixture(t, []*rspb.Release{rel}...) - // modify release status code & version - rel = newTestRelease(key, 2, rspb.Status_SUPERSEDED) + // modify release status code + rel.Info.Status.Code = rspb.Status_SUPERSEDED // perform the update - if err := cfgmaps.Update(rel); err != nil { + if err := cfgmaps.Update(key, rel); err != nil { t.Fatalf("Failed to update release: %s", err) } @@ -144,11 +150,8 @@ func TestConfigMapUpdate(t *testing.T) { } // check release has actually been updated by comparing modified fields - switch { - case rel.Info.Status.Code != got.Info.Status.Code: + if rel.Info.Status.Code != got.Info.Status.Code { t.Errorf("Expected status %s, got status %s", rel.Info.Status.Code, got.Info.Status.Code) - case rel.Version != got.Version: - t.Errorf("Expected version %d, got version %d", rel.Version, got.Version) } } @@ -177,11 +180,13 @@ func (mock *MockConfigMapsInterface) Init(t *testing.T, releases ...*rspb.Releas mock.objects = map[string]*api.ConfigMap{} for _, rls := range releases { - cfgmap, err := newConfigMapsObject(rls, nil) + objkey := testKey(rls.Name, rls.Version) + + cfgmap, err := newConfigMapsObject(objkey, rls, nil) if err != nil { t.Fatalf("Failed to create configmap: %s", err) } - mock.objects[rls.Name] = cfgmap + mock.objects[objkey] = cfgmap } } diff --git a/pkg/storage/driver/driver.go b/pkg/storage/driver/driver.go index f3593d278..96a096497 100644 --- a/pkg/storage/driver/driver.go +++ b/pkg/storage/driver/driver.go @@ -34,7 +34,7 @@ var ( // Create stores the release or returns ErrReleaseExists // if an identical release already exists. type Creator interface { - Create(rls *rspb.Release) error + Create(key string, rls *rspb.Release) error } // Updator is the interface that wraps the Update method. @@ -42,7 +42,7 @@ type Creator interface { // Update updates an existing release or returns // ErrReleaseNotFound if the release does not exist. type Updator interface { - Update(rls *rspb.Release) error + Update(key string, rls *rspb.Release) error } // Deletor is the interface that wraps the Delete method. diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index 76351b1a7..91e295f97 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -65,22 +65,22 @@ func (mem *Memory) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error } // Create creates a new release or returns ErrReleaseExists. -func (mem *Memory) Create(rls *rspb.Release) error { +func (mem *Memory) Create(key string, rls *rspb.Release) error { defer unlock(mem.wlock()) - if _, ok := mem.cache[rls.Name]; ok { + if _, ok := mem.cache[key]; ok { return ErrReleaseExists } - mem.cache[rls.Name] = rls + mem.cache[key] = rls return nil } // Update updates a release or returns ErrReleaseNotFound. -func (mem *Memory) Update(rls *rspb.Release) error { +func (mem *Memory) Update(key string, rls *rspb.Release) error { defer unlock(mem.wlock()) - if _, ok := mem.cache[rls.Name]; ok { - mem.cache[rls.Name] = rls + if _, ok := mem.cache[key]; ok { + mem.cache[key] = rls return nil } return ErrReleaseNotFound diff --git a/pkg/storage/driver/memory_test.go b/pkg/storage/driver/memory_test.go index 2c50fd1a4..bf49b4c19 100644 --- a/pkg/storage/driver/memory_test.go +++ b/pkg/storage/driver/memory_test.go @@ -17,6 +17,7 @@ limitations under the License. package driver // import "k8s.io/helm/pkg/storage/driver" import ( + "fmt" "reflect" "testing" @@ -33,11 +34,14 @@ func TestMemoryName(t *testing.T) { } func TestMemoryGet(t *testing.T) { - key := "test-1" - rls := &rspb.Release{Name: key} + vers := int32(1) + name := "smug-pigeon" + key := testKey(name, vers) + + rls := &rspb.Release{Name: name, Version: vers} mem := NewMemory() - if err := mem.Create(rls); err != nil { + if err := mem.Create(key, rls); err != nil { t.Fatalf("Failed create: %s", err) } @@ -45,46 +49,76 @@ func TestMemoryGet(t *testing.T) { if err != nil { t.Errorf("Could not get %s: %s", key, err) } - if res.Name != key { - t.Errorf("Expected %s, got %s", key, res.Name) + if res.Name != name { + t.Errorf("Expected name %s, got name %s", key, res.Name) + } + if res.Version != vers { + t.Errorf("Expected version %d, got version %d", vers, res.Version) } } func TestMemoryCreate(t *testing.T) { - key := "test-1" - rls := &rspb.Release{Name: key} + vers := int32(1) + name := "smug-pigeon" + key := testKey(name, vers) + + rls := &rspb.Release{Name: name, Version: vers} mem := NewMemory() - if err := mem.Create(rls); err != nil { + if err := mem.Create(key, rls); err != nil { t.Fatalf("Failed created: %s", err) } - if mem.cache[key].Name != key { + if mem.cache[key].Name != name { t.Errorf("Unexpected release name: %s", mem.cache[key].Name) } + if mem.cache[key].Version != vers { + t.Errorf("Unexpected release version: %d", mem.cache[key].Version) + } } func TestMemoryUpdate(t *testing.T) { - key := "test-1" - rls := &rspb.Release{Name: key} + vers := int32(1) + name := "smug-pigeon" + key := testKey(name, vers) + + rls := &rspb.Release{ + Name: name, + Version: vers, + Info: &rspb.Info{Status: &rspb.Status{Code: rspb.Status_DEPLOYED}}, + } mem := NewMemory() - if err := mem.Create(rls); err != nil { + if err := mem.Create(key, rls); err != nil { t.Fatalf("Failed create: %s", err) } - if err := mem.Update(rls); err != nil { + + // make an update to the release + rls.Info.Status.Code = rspb.Status_DELETED + + if err := mem.Update(key, rls); err != nil { t.Fatalf("Failed update: %s", err) } - if mem.cache[key].Name != key { + + if mem.cache[key].Name != name { t.Errorf("Unexpected release name: %s", mem.cache[key].Name) } + if mem.cache[key].Version != vers { + t.Errorf("Unexpected release version: %d", mem.cache[key].Version) + } + if mem.cache[key].Info.Status.Code != rspb.Status_DELETED { + t.Errorf("Unexpected status code: %s", mem.cache[key].Info.Status.Code) + } } func TestMemoryDelete(t *testing.T) { - key := "test-1" - rls := &rspb.Release{Name: key} + vers := int32(1) + name := "smug-pigeon" + key := testKey(name, vers) + + rls := &rspb.Release{Name: key, Version: vers} mem := NewMemory() - if err := mem.Create(rls); err != nil { + if err := mem.Create(key, rls); err != nil { t.Fatalf("Failed create: %s", err) } @@ -99,3 +133,7 @@ func TestMemoryDelete(t *testing.T) { t.Errorf("Expected %s, got %s", rls, res) } } + +func testKey(name string, version int32) string { + return fmt.Sprintf("%s#v%d", name, version) +} diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 85b204fcc..b086eca80 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -17,6 +17,7 @@ limitations under the License. package storage // import "k8s.io/helm/pkg/storage" import ( + "fmt" "log" rspb "k8s.io/helm/pkg/proto/hapi/release" @@ -30,10 +31,10 @@ type Storage struct { // Get retrieves the release from storage. An error is returned // if the storage driver failed to fetch the release, or the -// release identified by key does not exist. -func (s *Storage) Get(key string) (*rspb.Release, error) { - log.Printf("Getting release %q from storage\n", key) - return s.Driver.Get(key) +// release identified by the key, version pair does not exist. +func (s *Storage) Get(name string, version int32) (*rspb.Release, error) { + log.Printf("Getting release %q (v%d) from storage\n", name, version) + return s.Driver.Get(makeKey(name, version)) } // Create creates a new storage entry holding the release. An @@ -41,7 +42,7 @@ func (s *Storage) Get(key string) (*rspb.Release, error) { // release, or a release with identical an key already exists. func (s *Storage) Create(rls *rspb.Release) error { log.Printf("Create release %q in storage\n", rls.Name) - return s.Driver.Create(rls) + return s.Driver.Create(makeKey(rls.Name, rls.Version), rls) } // Update update the release in storage. An error is returned if the @@ -49,15 +50,15 @@ func (s *Storage) Create(rls *rspb.Release) error { // does not exist. func (s *Storage) Update(rls *rspb.Release) error { log.Printf("Updating %q in storage\n", rls.Name) - return s.Driver.Update(rls) + return s.Driver.Update(makeKey(rls.Name, rls.Version), rls) } // Delete deletes the release from storage. An error is returned if // the storage backend fails to delete the release or if the release // does not exist. -func (s *Storage) Delete(key string) (*rspb.Release, error) { - log.Printf("Deleting release %q from storage\n", key) - return s.Driver.Delete(key) +func (s *Storage) Delete(name string, version int32) (*rspb.Release, error) { + log.Printf("Deleting release %q (v%d) from storage\n", name, version) + return s.Driver.Delete(makeKey(name, version)) } // ListReleases returns all releases from storage. An error is returned if the @@ -114,3 +115,10 @@ func Init(d driver.Driver) *Storage { } return &Storage{Driver: d} } + +// makeKey concatenates a release name and version into +// a string with format ```#v```. +// This key is used to uniquely identify storage objects. +func makeKey(rlsname string, version int32) string { + return fmt.Sprintf("%s#v%d", rlsname, version) +} diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go index 25975bf1b..8ac1e00a1 100644 --- a/pkg/storage/storage_test.go +++ b/pkg/storage/storage_test.go @@ -30,11 +30,12 @@ func TestStorageCreate(t *testing.T) { storage := Init(driver.NewMemory()) // create fake release - rls := ReleaseTestData{Name: "angry-beaver"}.ToRelease() + rls := ReleaseTestData{Name: "angry-beaver", Version: 1}.ToRelease() + assertErrNil(t.Fatal, storage.Create(rls), "StoreRelease") // fetch the release - res, err := storage.Get(rls.Name) + res, err := storage.Get(rls.Name, rls.Version) assertErrNil(t.Fatal, err, "QueryRelease") // verify the fetched and created release are the same @@ -48,7 +49,8 @@ func TestStorageUpdate(t *testing.T) { storage := Init(driver.NewMemory()) // create fake release - rls := ReleaseTestData{Name: "angry-beaver"}.ToRelease() + rls := ReleaseTestData{Name: "angry-beaver", Version: 1}.ToRelease() + assertErrNil(t.Fatal, storage.Create(rls), "StoreRelease") // modify the release @@ -57,7 +59,7 @@ func TestStorageUpdate(t *testing.T) { assertErrNil(t.Fatal, storage.Update(rls), "UpdateRelease") // retrieve the updated release - res, err := storage.Get(rls.Name) + res, err := storage.Get(rls.Name, rls.Version) assertErrNil(t.Fatal, err, "QueryRelease") // verify updated and fetched releases are the same. @@ -71,11 +73,11 @@ func TestStorageDelete(t *testing.T) { storage := Init(driver.NewMemory()) // create fake release - rls := ReleaseTestData{Name: "angry-beaver"}.ToRelease() + rls := ReleaseTestData{Name: "angry-beaver", Version: 1}.ToRelease() assertErrNil(t.Fatal, storage.Create(rls), "StoreRelease") // delete the release - res, err := storage.Delete(rls.Name) + res, err := storage.Delete(rls.Name, rls.Version) assertErrNil(t.Fatal, err, "DeleteRelease") // verify updated and fetched releases are the same.