Addressing review issues

api/machines.go

@@ -17,7 +17,6 @@ package api
 import (
 	"encoding/json"
 	"errors"
-	"fmt"
 	"net/http"
 	"path"
 	"regexp"
@@ -29,7 +28,7 @@ import (
 )
 
 var (
-	metadataPathRegex = regexp.MustCompile("^/([0-9a-f]{32})/metadata/([A-Za-z0-9_.-]+$)")
+	metadataPathRegex = regexp.MustCompile("^/([^/]+)/metadata/([A-Za-z0-9_.-]+$)")
 )
 
 func wireUpMachinesResource(mux *http.ServeMux, prefix string, cAPI client.API) {
@@ -43,9 +42,9 @@ type machinesResource struct {
 }
 
 type machineMetadataOp struct {
-	Op    string
-	Path  string
-	Value string
+	Operation string `json:"op"`
+	Path      string
+	Value     string
 }
 
 func (mr *machinesResource) ServeHTTP(rw http.ResponseWriter, req *http.Request) {
@@ -83,55 +82,50 @@ func (mr *machinesResource) list(rw http.ResponseWriter, req *http.Request) {
 
 func (mr *machinesResource) patch(rw http.ResponseWriter, req *http.Request) {
 	ops := make([]machineMetadataOp, 0)
-
 	dec := json.NewDecoder(req.Body)
-
 	if err := dec.Decode(&ops); err != nil {
-		sendError(rw, http.StatusInternalServerError, err)
+		sendError(rw, http.StatusBadRequest, err)
 		return
 	}
 
-	// Validate all operations before applying
 	for _, op := range ops {
-		if op.Op != "add" && op.Op != "remove" && op.Op != "replace" {
-			sendError(rw, http.StatusBadRequest, fmt.Errorf("Bad op: %s. Expected add/remove/replace", op.Op))
+		if op.Operation != "add" && op.Operation != "remove" && op.Operation != "replace" {
+			sendError(rw, http.StatusBadRequest, errors.New("invalid op: expect add, remove, or replace"))
 			return
 		}
 
-		s := metadataPathRegex.FindStringSubmatch(op.Path)
-		if s == nil {
-			sendError(rw, http.StatusBadRequest, fmt.Errorf("Bad path: %s. Expected {machine id}/metadata/{key}. Key must only contain [a-zA-Z0-9_.-]", op.Path))
+		if metadataPathRegex.FindStringSubmatch(op.Path) == nil {
+			sendError(rw, http.StatusBadRequest, errors.New("machine metadata path invalid"))
 			return
 		}
 
-		if op.Op != "remove" && len(op.Value) == 0 {
-			sendError(rw, http.StatusBadRequest, fmt.Errorf("Path %s has no value", op.Path))
+		if op.Operation != "remove" && len(op.Value) == 0 {
+			sendError(rw, http.StatusBadRequest, errors.New("invalid value: add and replace require a value"))
 			return
 		}
 	}
 
-	// Apply metadata changes
 	for _, op := range ops {
 		// regex already validated above
 		s := metadataPathRegex.FindStringSubmatch(op.Path)
 		machID := s[1]
 		key := s[2]
 
-		if op.Op == "remove" {
+		if op.Operation == "remove" {
 			err := mr.cAPI.DeleteMachineMetadata(machID, key)
 			if err != nil {
 				sendError(rw, http.StatusInternalServerError, err)
 				return
 			}
 		} else {
-			err := mr.cAPI.UpdateMachineMetadata(machID, key, op.Value)
+			err := mr.cAPI.SetMachineMetadata(machID, key, op.Value)
 			if err != nil {
 				sendError(rw, http.StatusInternalServerError, err)
 				return
 			}
 		}
 	}
-	sendResponse(rw, http.StatusOK, "")
+	sendResponse(rw, http.StatusNoContent, "")
 }
 
 func getMachinePage(cAPI client.API, tok PageToken) (*schema.MachinePage, error) {

api/machines_test.go

@@ -30,8 +30,8 @@ import (
 func fakeMachinesSetup() (*machinesResource, *httptest.ResponseRecorder) {
 	fr := registry.NewFakeRegistry()
 	fr.SetMachines([]machine.MachineState{
-		{ID: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", PublicIP: "", Metadata: map[string]string{}},
-		{ID: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", PublicIP: "1.2.3.4", Metadata: map[string]string{"ping": "pong"}},
+		{ID: "XXX", PublicIP: "", Metadata: map[string]string{}},
+		{ID: "YYY", PublicIP: "1.2.3.4", Metadata: map[string]string{"ping": "pong"}},
 	})
 	fAPI := &client.RegistryClient{Registry: fr}
 	resource := &machinesResource{cAPI: fAPI}
@@ -63,7 +63,7 @@ func TestMachinesList(t *testing.T) {
 		t.Error("Received nil response body")
 	} else {
 		body := rw.Body.String()
-		expected := `{"machines":[{"id":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"},{"id":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","metadata":{"ping":"pong"},"primaryIP":"1.2.3.4"}]}`
+		expected := `{"machines":[{"id":"XXX"},{"id":"YYY","metadata":{"ping":"pong"},"primaryIP":"1.2.3.4"}]}`
 		if body != expected {
 			t.Errorf("Expected body:\n%s\n\nReceived body:\n%s\n", expected, body)
 		}
@@ -158,8 +158,8 @@ func TestExtractMachinePage(t *testing.T) {
 
 func TestMachinesPatchAddModify(t *testing.T) {
 	reqBody := `
-	[{"op": "add", "path": "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/metadata/foo", "value": "bar"},
-	 {"op": "replace", "path": "/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/metadata/ping", "value": "splat"}]
+	[{"op": "add", "path": "/XXX/metadata/foo", "value": "bar"},
+	 {"op": "replace", "path": "/YYY/metadata/ping", "value": "splat"}]
 	`
 
 	resource, rw := fakeMachinesSetup()
@@ -169,8 +169,8 @@ func TestMachinesPatchAddModify(t *testing.T) {
 	}
 
 	resource.ServeHTTP(rw, req)
-	if rw.Code != http.StatusOK {
-		t.Errorf("Expected 200, got %d", rw.Code)
+	if rw.Code != http.StatusNoContent {
+		t.Errorf("Expected 204, got %d", rw.Code)
 	}
 
 	// fetch machine to make sure data has been added
@@ -185,7 +185,7 @@ func TestMachinesPatchAddModify(t *testing.T) {
 		t.Error("Received nil response body")
 	} else {
 		body := rw.Body.String()
-		expected := `{"machines":[{"id":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa","metadata":{"foo":"bar"}},{"id":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","metadata":{"ping":"splat"},"primaryIP":"1.2.3.4"}]}`
+		expected := `{"machines":[{"id":"XXX","metadata":{"foo":"bar"}},{"id":"YYY","metadata":{"ping":"splat"},"primaryIP":"1.2.3.4"}]}`
 		if body != expected {
 			t.Errorf("Expected body:\n%s\n\nReceived body:\n%s\n", expected, body)
 		}
@@ -194,8 +194,8 @@ func TestMachinesPatchAddModify(t *testing.T) {
 
 func TestMachinesPatchDelete(t *testing.T) {
 	reqBody := `
-	[{"op": "remove", "path": "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/metadata/foo"},
-	 {"op": "remove", "path": "/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/metadata/ping"}]
+	[{"op": "remove", "path": "/XXX/metadata/foo"},
+	 {"op": "remove", "path": "/YYY/metadata/ping"}]
 	`
 
 	resource, rw := fakeMachinesSetup()
@@ -205,8 +205,8 @@ func TestMachinesPatchDelete(t *testing.T) {
 	}
 
 	resource.ServeHTTP(rw, req)
-	if rw.Code != http.StatusOK {
-		t.Errorf("Expected 200, got %d", rw.Code)
+	if rw.Code != http.StatusNoContent {
+		t.Errorf("Expected 204, got %d", rw.Code)
 	}
 
 	// fetch machine to make sure data has been added
@@ -221,7 +221,7 @@ func TestMachinesPatchDelete(t *testing.T) {
 		t.Error("Received nil response body")
 	} else {
 		body := rw.Body.String()
-		expected := `{"machines":[{"id":"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"},{"id":"bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb","primaryIP":"1.2.3.4"}]}`
+		expected := `{"machines":[{"id":"XXX"},{"id":"YYY","primaryIP":"1.2.3.4"}]}`
 		if body != expected {
 			t.Errorf("Expected body:\n%s\n\nReceived body:\n%s\n", expected, body)
 		}
@@ -230,7 +230,7 @@ func TestMachinesPatchDelete(t *testing.T) {
 
 func TestMachinesPatchBadOp(t *testing.T) {
 	reqBody := `
-	[{"op": "noop", "path": "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/metadata/foo", "value": "bar"}]
+	[{"op": "noop", "path": "/XXX/metadata/foo", "value": "bar"}]
 	`
 
 	resource, rw := fakeMachinesSetup()
@@ -247,7 +247,7 @@ func TestMachinesPatchBadOp(t *testing.T) {
 
 func TestMachinesPatchBadPath(t *testing.T) {
 	reqBody := `
-	[{"op": "add", "path": "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/foo", "value": "bar"}]
+	[{"op": "add", "path": "/XXX/foo", "value": "bar"}]
 	`
 
 	resource, rw := fakeMachinesSetup()
@@ -264,7 +264,7 @@ func TestMachinesPatchBadPath(t *testing.T) {
 
 func TestMachinesPatchBadValue(t *testing.T) {
 	reqBody := `
-	[{"op": "add", "path": "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/foo"}]
+	[{"op": "add", "path": "/XXX/foo"}]
 	`
 
 	resource, rw := fakeMachinesSetup()

client/api.go

@@ -21,7 +21,7 @@ import (
 
 type API interface {
 	Machines() ([]machine.MachineState, error)
-	UpdateMachineMetadata(machID string, key string, value string) error
+	SetMachineMetadata(machID string, key string, value string) error
 	DeleteMachineMetadata(machID string, key string) error
 
 	Unit(string) (*schema.Unit, error)

registry/fake.go

@@ -279,7 +279,7 @@ func (f *FakeRegistry) UnitHeartbeat(name, machID string, ttl time.Duration) err
 
 func (f *FakeRegistry) ClearUnitHeartbeat(string) {}
 
-func (f *FakeRegistry) UpdateMachineMetadata(machID string, key string, value string) error {
+func (f *FakeRegistry) SetMachineMetadata(machID string, key string, value string) error {
 	for _, mach := range f.machines {
 		if mach.ID == machID {
 			mach.Metadata[key] = value

registry/interface.go

@@ -37,8 +37,8 @@ type Registry interface {
 	SetUnitTargetState(name string, state job.JobState) error
 	SetMachineState(ms machine.MachineState, ttl time.Duration) (uint64, error)
 	UnscheduleUnit(name, machID string) error
-	UpdateMachineMetadata(machID string, key string, value string) error
-	DeleteMachineMetadata(machID string, key string) error
+	SetMachineMetadata(machID, key, value string) error
+	DeleteMachineMetadata(machID, key string) error
 
 	UnitRegistry
 }

registry/machine.go

@@ -45,8 +45,7 @@ func (r *EtcdRegistry) Machines() (machines []machine.MachineState, err error) {
 	for _, node := range resp.Node.Nodes {
 		var mach machine.MachineState
 
-		_, machineID := path.Split(node.Key)
-		mach.ID = machineID
+		mach.ID = path.Base(node.Key)
 
 		for _, obj := range node.Nodes {
 			// Load machine object
@@ -57,10 +56,9 @@ func (r *EtcdRegistry) Machines() (machines []machine.MachineState, err error) {
 				}
 			} else if strings.HasSuffix(obj.Key, "/metadata") {
 				// Load metadata
-				mach.Metadata = make(map[string]string)
+				mach.Metadata = make(map[string]string, len(obj.Nodes))
 				for _, mdnode := range obj.Nodes {
-					_, key := path.Split(mdnode.Key)
-					mach.Metadata[key] = mdnode.Value
+					mach.Metadata[path.Base(mdnode.Key)] = mdnode.Value
 				}
 			}
 		}
@@ -105,37 +103,22 @@ func (r *EtcdRegistry) SetMachineState(ms machine.MachineState, ttl time.Duratio
 	}
 
 	// Set the initial metadata on creation
-	if err = r.setMachineMetadata(ms); err != nil {
+	if err = r.setMachineMetadata(ms.ID, ms.Metadata); err != nil {
 		return uint64(0), err
 	}
 
 	return resp.Node.ModifiedIndex, nil
 }
 
-func (r *EtcdRegistry) UpdateMachineMetadata(machID string, key string, value string) error {
+func (r *EtcdRegistry) SetMachineMetadata(machID string, key string, value string) error {
 	//Attempt to update key
-	update := etcd.Update{
+	update := etcd.Set{
 		Key:   path.Join(r.keyPrefix, machinePrefix, machID, "metadata", key),
 		Value: value,
 	}
 
 	_, err := r.etcd.Do(&update)
-	if err == nil {
-		return nil
-	}
-
-	// Create
-	create := etcd.Create{
-		Key:   path.Join(r.keyPrefix, machinePrefix, machID, "metadata", key),
-		Value: value,
-	}
-
-	_, err = r.etcd.Do(&create)
-	if err != nil {
-		return err
-	}
-
-	return nil
+	return err
 }
 
 func (r *EtcdRegistry) DeleteMachineMetadata(machID string, key string) error {
@@ -145,17 +128,15 @@ func (r *EtcdRegistry) DeleteMachineMetadata(machID string, key string) error {
 
 	_, err := r.etcd.Do(&del)
 	if etcd.IsKeyNotFound(err) {
-		return nil
-	} else if err != nil {
-		return err
+		err = nil
 	}
-	return nil
+	return err
 }
 
 // Remove and reset all metadata
-func (r *EtcdRegistry) setMachineMetadata(ms machine.MachineState) error {
+func (r *EtcdRegistry) setMachineMetadata(machID string, metadata map[string]string) error {
 	del := etcd.Delete{
-		Key:       path.Join(r.keyPrefix, machinePrefix, ms.ID, "metadata"),
+		Key:       path.Join(r.keyPrefix, machinePrefix, machID, "metadata"),
 		Recursive: true,
 	}
 
@@ -168,8 +149,8 @@ func (r *EtcdRegistry) setMachineMetadata(ms machine.MachineState) error {
 		return err
 	}
 
-	for key, val := range ms.Metadata {
-		if err = r.UpdateMachineMetadata(ms.ID, key, val); err != nil {
+	for key, val := range metadata {
+		if err = r.SetMachineMetadata(machID, key, val); err != nil {
 			return err
 		}
 	}