From 559e9081fb8b93ff61d21d7506d413ca242f7207 Mon Sep 17 00:00:00 2001 From: fibonacci1729 Date: Tue, 9 Aug 2016 12:49:31 -0600 Subject: [PATCH] style fixes and cleanup --- cmd/tiller/environment/environment.go | 4 +- pkg/storage/doc.go | 10 ++--- pkg/storage/driver/cfgmaps.go | 59 +++++++++++++++++---------- pkg/storage/driver/cfgmaps_test.go | 4 +- pkg/storage/driver/driver.go | 3 +- pkg/storage/driver/memory.go | 2 +- pkg/storage/driver/memory_test.go | 42 +++++++++---------- pkg/storage/storage.go | 3 +- 8 files changed, 69 insertions(+), 58 deletions(-) diff --git a/cmd/tiller/environment/environment.go b/cmd/tiller/environment/environment.go index 61d2650aa..c40e37a73 100644 --- a/cmd/tiller/environment/environment.go +++ b/cmd/tiller/environment/environment.go @@ -33,10 +33,10 @@ import ( "k8s.io/helm/pkg/storage/driver" ) -// Feature flags for configmaps storage driver +// UseConfigMaps is a feature flags to toggle use of configmaps storage driver. const UseConfigMaps = false -// Tiller's namespace +// TillerNamespace is the namespace tiller is running in. const TillerNamespace = "kube-system" // GoTplEngine is the name of the Go template engine, as registered in the EngineYard. diff --git a/pkg/storage/doc.go b/pkg/storage/doc.go index 2a3cf3ce0..231e30bb9 100644 --- a/pkg/storage/doc.go +++ b/pkg/storage/doc.go @@ -14,11 +14,9 @@ See the License for the specific language governing permissions and limitations under the License. */ -/*Package storage implements storage for Tiller objects. - -Tiller stores releases (see 'cmd/tiller/environment'.Environment). The backend -storage mechanism may be implemented with different backends. This package -and its subpackages provide storage layers for Tiller objects. +/* +Package storage implements storage for Tiller objects.The backend storage +mechanism may be implemented with different backends. This package and its +subpackages provide storage layers for Tiller objects. */ - package storage // import "k8s.io/helm/pkg/storage" diff --git a/pkg/storage/driver/cfgmaps.go b/pkg/storage/driver/cfgmaps.go index fab8bf15a..8807eb97b 100644 --- a/pkg/storage/driver/cfgmaps.go +++ b/pkg/storage/driver/cfgmaps.go @@ -34,6 +34,7 @@ import ( var b64 = base64.StdEncoding +// labels is a map of key value pairs to be included as metadata in a configmap object. type labels map[string]string func (lbs *labels) init() { *lbs = labels(make(map[string]string)) } @@ -72,13 +73,12 @@ func (cfgmaps *ConfigMaps) Get(key string) (*rspb.Release, error) { logerrf(err, "get: failed to decode data %q", key) return nil, err } - // return the release object return r, nil } -// List fetches all releases and returns a list of all releases -// where filter(release) == true. An error is returned if the +// List fetches all releases and returns the list releases such +// that filter(release) == true. An error is returned if the // configmap fails to retrieve the releases. func (cfgmaps *ConfigMaps) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error) { list, err := cfgmaps.impl.List(api.ListOptions{}) @@ -89,6 +89,8 @@ func (cfgmaps *ConfigMaps) List(filter func(*rspb.Release) bool) ([]*rspb.Releas var results []*rspb.Release + // iterate over the configmaps object list + // and decode each release for _, item := range list.Items { rls, err := decodeRelease(item.Data["release"]) if err != nil { @@ -99,19 +101,20 @@ func (cfgmaps *ConfigMaps) List(filter func(*rspb.Release) bool) ([]*rspb.Releas results = append(results, rls) } } - return results, nil } +// Create creates a new ConfigMap holding the release. If the +// ConfigMap already exists, ErrReleaseExists is returned. func (cfgmaps *ConfigMaps) Create(rls *rspb.Release) error { + // set labels for configmaps object meta data var lbs labels - // set labels for configmaps object meta data lbs.init() lbs.set("STATE", "CREATED") lbs.set("CREATED_AT", time.Now().String()) - // create a new configmap object from the release + // create a new configmap to hold the release obj, err := newConfigMapsObject(rls, lbs) if err != nil { logerrf(err, "create: failed to encode release %q", rls.Name) @@ -126,15 +129,21 @@ func (cfgmaps *ConfigMaps) Create(rls *rspb.Release) error { logerrf(err, "create: failed to create") return err } - return nil } // 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 { - // create a new configmap object from the release - obj, err := newConfigMapsObject(rls, labels{"MODIFIED_AT": time.Now().String()}) + // set labels for configmaps object meta data + var lbs labels + + lbs.init() + lbs.set("STATE", "UPDATED") + lbs.set("MODIFIED_AT", time.Now().String()) + + // create a new configmap object to hold the release + obj, err := newConfigMapsObject(rls, lbs) if err != nil { logerrf(err, "update: failed to encode release %q", rls.Name) return err @@ -145,12 +154,18 @@ func (cfgmaps *ConfigMaps) Update(rls *rspb.Release) error { logerrf(err, "update: failed to update") return err } - return nil } // Delete deletes the ConfigMap holding the release named by key. func (cfgmaps *ConfigMaps) Delete(key string) (rls *rspb.Release, err error) { + // set labels for configmaps object meta data + var lbs labels + + lbs.init() + lbs.set("STATE", "DELETED") + lbs.set("MODIFIED_AT", time.Now().String()) + // fetch the release to check existence if rls, err = cfgmaps.Get(key); err != nil { if kberrs.IsNotFound(err) { @@ -164,17 +179,17 @@ func (cfgmaps *ConfigMaps) Delete(key string) (rls *rspb.Release, err error) { if err = cfgmaps.impl.Delete(key); err != nil { return rls, err } - return + return rls, nil } // newConfigMapsObject constructs a kubernetes ConfigMap object -// from a release. Each configmap data entry is the base64 encoded -// string of a release's binary protobuf encoding. +// to store a release. Each configmap data entry is the base64 +// encoded string of a release's binary protobuf encoding. // // The following labels are used within each configmap: // -// "LAST_MODIFIED" - timestamp indicating when this configmap was last modified. (set in Update) -// "CREATED_AT" - timestamp indicating when this configmap was created. (set in Create) +// "MODIFIED_AT" - timestamp indicating when this configmap was last modified. (set in Update) +// "CREATED_AT" - timestamp indicating when this configmap was created. (set in Create) // "VERSION" - version of the release. // "OWNER" - owner of the configmap, currently "TILLER". // "NAME" - name of the release. @@ -189,9 +204,10 @@ func newConfigMapsObject(rls *rspb.Release, lbs labels) (*api.ConfigMap, error) } if lbs == nil { - lbs = labels{} + lbs.init() } + // apply labels lbs.set("NAME", rls.Name) lbs.set("OWNER", owner) lbs.set("VERSION", strconv.Itoa(int(rls.Version))) @@ -206,8 +222,8 @@ func newConfigMapsObject(rls *rspb.Release, lbs labels) (*api.ConfigMap, error) }, nil } -// encodeRelease encodes a release returning a base64 encoded binary protobuf -// encoding representation, or error. +// encodeRelease encodes a release returning a base64 encoded +// binary protobuf encoding representation, or error. func encodeRelease(rls *rspb.Release) (string, error) { b, err := proto.Marshal(rls) if err != nil { @@ -217,8 +233,8 @@ func encodeRelease(rls *rspb.Release) (string, error) { } // decodeRelease decodes the bytes in data into a release -// type. Data must contain a valid base64 encoded string -// of a valid protobuf encoding of a release, otherwise +// type. Data must contain a base64 encoded string of a +// valid protobuf encoding of a release, otherwise // an error is returned. func decodeRelease(data string) (*rspb.Release, error) { // base64 decode string @@ -232,11 +248,10 @@ func decodeRelease(data string) (*rspb.Release, error) { if err := proto.Unmarshal(b, &rls); err != nil { return nil, err } - return &rls, nil } -// for debugging +// logerrf wraps an error with the a formatted string (used for debugging) func logerrf(err error, format string, args ...interface{}) { log.Printf("configmaps: %s: %s\n", fmt.Sprintf(format, args...), err) } diff --git a/pkg/storage/driver/cfgmaps_test.go b/pkg/storage/driver/cfgmaps_test.go index dd032258c..64e836dd4 100644 --- a/pkg/storage/driver/cfgmaps_test.go +++ b/pkg/storage/driver/cfgmaps_test.go @@ -59,11 +59,11 @@ func TestConfigMapCreate(t *testing.T) { // store the release in a configmap if err := cfgmaps.Create(rls); err != nil { - t.Fatalf("failed to create release: %s", key, err) + t.Fatalf("failed to create release with key %q: %s", key, err) } if err := cfgmaps.Create(rls); err != nil { - t.Fatalf("failed to create release: %s", key, err) + t.Fatalf("failed to create release with key %q: %s", key, err) } // get the release back diff --git a/pkg/storage/driver/driver.go b/pkg/storage/driver/driver.go index fc833aaf6..0cf51d6f6 100644 --- a/pkg/storage/driver/driver.go +++ b/pkg/storage/driver/driver.go @@ -18,6 +18,7 @@ package driver // import "k8s.io/helm/pkg/storage/driver" import ( "errors" + rspb "k8s.io/helm/pkg/proto/hapi/release" ) @@ -26,8 +27,6 @@ var ( ErrReleaseNotFound = errors.New("release: not found") // ErrReleaseExists indicates that a release already exists. ErrReleaseExists = errors.New("release: already exists") - // ErrDriverAction indicates the storage driver failed to execute the requested action. - ErrDriverAction = errors.New("driver: failed to perform action") ) // Creator is the interface that wraps the Create method. diff --git a/pkg/storage/driver/memory.go b/pkg/storage/driver/memory.go index 85918a68c..bea495133 100644 --- a/pkg/storage/driver/memory.go +++ b/pkg/storage/driver/memory.go @@ -43,7 +43,7 @@ func (mem *Memory) Get(key string) (*rspb.Release, error) { return nil, ErrReleaseNotFound } -// List returns all releases whose status is not Status_DELETED. +// List returns the list of all releases such that filter(release) == true func (mem *Memory) List(filter func(*rspb.Release) bool) ([]*rspb.Release, error) { defer unlock(mem.rlock()) diff --git a/pkg/storage/driver/memory_test.go b/pkg/storage/driver/memory_test.go index 91b69a274..b02f8350b 100644 --- a/pkg/storage/driver/memory_test.go +++ b/pkg/storage/driver/memory_test.go @@ -17,8 +17,10 @@ limitations under the License. package driver // import "k8s.io/helm/pkg/storage/driver" import ( - rspb "k8s.io/helm/pkg/proto/hapi/release" + "reflect" "testing" + + rspb "k8s.io/helm/pkg/proto/hapi/release" ) func TestMemoryGet(t *testing.T) { @@ -26,13 +28,15 @@ func TestMemoryGet(t *testing.T) { rls := &rspb.Release{Name: key} mem := NewMemory() - mem.Create(rls) + if err := mem.Create(rls); err != nil { + t.Fatalf("Failed create: %s", err) + } res, err := mem.Get(key) - switch { - case err != nil: + if err != nil { t.Errorf("Could not get %s: %s", key, err) - case res.Name != key: + } + if res.Name != key { t.Errorf("Expected %s, got %s", key, res.Name) } } @@ -42,12 +46,10 @@ func TestMemoryCreate(t *testing.T) { rls := &rspb.Release{Name: key} mem := NewMemory() - err := mem.Create(rls) - - switch { - case err != nil: - t.Fatalf("Failed create: %s", err) - case mem.cache[key].Name != key: + if err := mem.Create(rls); err != nil { + t.Fatalf("Failed created: %s", err) + } + if mem.cache[key].Name != key { t.Errorf("Unexpected release name: %s", mem.cache[key].Name) } } @@ -70,12 +72,7 @@ func TestMemoryUpdate(t *testing.T) { func TestMemoryDelete(t *testing.T) { key := "test-1" - rls := &rspb.Release{ - Name: key, - Info: &rspb.Info{ - Status: &rspb.Status{Code: rspb.Status_DELETED}, - }, - } + rls := &rspb.Release{Name: key} mem := NewMemory() if err := mem.Create(rls); err != nil { @@ -83,12 +80,13 @@ func TestMemoryDelete(t *testing.T) { } res, err := mem.Delete(key) - switch { - case err != nil: + if err != nil { t.Fatalf("Failed delete: %s", err) - case mem.cache[key] != nil: + } + if mem.cache[key] != nil { t.Errorf("Expected nil, got %s", mem.cache[key]) - case res.Info.Status.Code != rspb.Status_DELETED: - t.Errorf("Expected Status_DELETED, got %s", res.Info.Status.Code) + } + if !reflect.DeepEqual(rls, res) { + t.Errorf("Expected %s, got %s", rls, res) } } diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go index 0204249fd..85b204fcc 100644 --- a/pkg/storage/storage.go +++ b/pkg/storage/storage.go @@ -17,9 +17,10 @@ limitations under the License. package storage // import "k8s.io/helm/pkg/storage" import ( + "log" + rspb "k8s.io/helm/pkg/proto/hapi/release" "k8s.io/helm/pkg/storage/driver" - "log" ) // Storage represents a storage engine for a Release.