style fixes and cleanup

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.

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"

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)
 }

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

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.

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())
 

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)
 	}
 }

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.