Merge lp:~dave-cheney/juju-core/go-cmd-juju-status-plus-plus into lp:~juju/juju-core/trunk

[Dave Cheney]

cmd/juju: status: gather unit information

Add basic support for units, and some cleanups.

https://codereview.appspot.com/6452056/

[Gustavo Niemeyer (niemeyer)]

LGTM

https://codereview.appspot.com/6452056/diff/2001/cmd/juju/status.go
File cmd/juju/status.go (right):

https://codereview.appspot.com/6452056/diff/2001/cmd/juju/status.go#newcode216
cmd/juju/status.go:216: // TODO(dfc) we could make this nicer, ie
machine/0
I'd do "m-0", but unfortunately it's a bit late. :-(

https://codereview.appspot.com/6452056/diff/2001/cmd/juju/status.go#newcode225
cmd/juju/status.go:225: AgentVersion() (version.Version, error)
Same comment regarding winning the race.

https://codereview.appspot.com/6452056/diff/2001/cmd/juju/status.go#newcode264
cmd/juju/status.go:264: func chkError(m map[string]interface{}, err
error) map[string]interface{} {
s/chkError/checkError/

https://codereview.appspot.com/6452056/

[lp:~dave-cheney/juju-core/go-cmd-juju-status-plus-plus]

351. By Dave Cheney on 2012-08-02

merge from trunk

Unmerged revisions

351. By Dave Cheney on 2012-08-02

merge from trunk

350. By Dave Cheney on 2012-07-27

cleanup

349. By Dave Cheney on 2012-07-27

added units

348. By Dave Cheney on 2012-07-27

added error checking

347. By Roger Peppe on 2012-07-27

container: add ToolsDir method

This allows the machine agent to copy tools into the correct directory
for a unit.

R=niemeyer
CC=
https://codereview.appspot.com/6441059

346. By William Reade on 2012-07-26

add new methods to RelationUnit

Endpoint is required to construct JUJU_RELATION for the unit agent
Relation is also required to construct JUJU_RELATION_ID for the unit agent
ReadSettings will be used by the unit agent to read settings for departed
units, and occasionally to populate cold settings caches.

R=niemeyer
CC=
https://codereview.appspot.com/6440050

345. By Roger Peppe on 2012-07-26

state: embed agentVersion into Unit

R=niemeyer
CC=
https://codereview.appspot.com/6449050

344. By Roger Peppe on 2012-07-26

state: add an agentVersion type and embed it into Machine.

R=niemeyer
CC=
https://codereview.appspot.com/6441053

343. By Roger Peppe on 2012-07-26

state: factor out getConfigString and setConfigString

This code was repeated in quite a few places with minor (and
IMHO mostly unjustified) variations. We're about to add more
calls (version and proposed-version), which hopefully should
justify this even more.

R=niemeyer
CC=
https://codereview.appspot.com/6442051

342. By Roger Peppe on 2012-07-26

state: move RelationScope type to charm.

Also make the schema error for OneOf a little
more useful.

R=niemeyer
CC=
https://codereview.appspot.com/6449043

=== modified file 'cmd/juju/status.go'
--- cmd/juju/status.go	2012-08-02 22:54:18 +0000
+++ cmd/juju/status.go	2012-08-02 22:54:18 +0000
@@ -9,6 +9,7 @@
 	"launchpad.net/juju-core/environs"
 	"launchpad.net/juju-core/juju"
 	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/version"
 )
 
 type StatusCommand struct {
@@ -63,7 +64,7 @@
 		return err
 	}
 
-	result := make(map[string]interface{})
+	result := m()
 
 	result["machines"], err = processMachines(machines, instances)
 	if err != nil {
@@ -141,38 +142,22 @@
 				// yet the environ cannot find that id. 
 				return nil, fmt.Errorf("instance %s for machine %d not found", instid, m.Id())
 			}
-			machine, err := processMachine(m, instance)
-			if err != nil {
-				machine = map[string]interface{}{
-					"status-error": err.Error(),
-				}
-			}
-			r[m.Id()] = machine
+			r[m.Id()] = chkError(processMachine(m, instance))
 		}
 	}
 	return r, nil
 }
 
 func processMachine(machine *state.Machine, instance environs.Instance) (map[string]interface{}, error) {
-	r := make(map[string]interface{})
+	r := m()
 	r["instance-id"] = instance.Id()
 
 	if dnsname, err := instance.DNSName(); err == nil {
 		r["dns-name"] = dnsname
 	}
 
-	if v, err := machine.AgentVersion(); err == nil {
-		r["agent-version"] = v.String()
-	}
-
-	if v, err := machine.ProposedAgentVersion(); err == nil {
-		r["proposed-agent-version"] = v.String()
-	}
-
-	if alive, err := machine.AgentAlive(); err == nil && alive {
-		// TODO(dfc) revisit this once unit-status is done
-		r["agent-state"] = "running"
-	}
+	processVersion(r, machine)
+	processAgentStatus(r, machine)
 
 	// TODO(dfc) unit-status
 	return r, nil
@@ -180,32 +165,88 @@
 
 // processServices gathers information about services.
 func processServices(services map[string]*state.Service) (map[string]interface{}, error) {
-	r := make(map[string]interface{})
+	r := m()
 	for _, s := range services {
-		service, err := processService(s)
-		if err != nil {
-			service = map[string]interface{}{
-				"status-error": err.Error(),
-			}
-		}
-		r[s.Name()] = service
+		r[s.Name()] = chkError(processService(s))
 	}
 	return r, nil
 }
 
 func processService(service *state.Service) (map[string]interface{}, error) {
-	r := make(map[string]interface{})
+	r := m()
 	ch, err := service.Charm()
 	if err != nil {
 		return nil, err
 	}
 	r["charm"] = ch.Meta().Name
-	r["exposed"], err = service.IsExposed()
+
+	if exposed, err := service.IsExposed(); err == nil {
+		r["exposed"] = exposed
+	}
+
+	// TODO(dfc) service.IsSubordinate() ?
+
+	units, err := service.AllUnits()
 	if err != nil {
 		return nil, err
 	}
-	// TODO(dfc) process units and relations
-	return r, nil
+
+	u := chkError(processUnits(units))
+	if len(u) > 0 {
+		r["units"] = u
+	}
+
+	// TODO(dfc) process relations
+	return r, nil
+}
+
+func processUnits(units []*state.Unit) (map[string]interface{}, error) {
+	r := m()
+	for _, unit := range units {
+		r[unit.Name()] = chkError(processUnit(unit))
+	}
+	return r, nil
+}
+
+func processUnit(unit *state.Unit) (map[string]interface{}, error) {
+	r := m()
+
+	if addr, err := unit.PublicAddress(); err == nil {
+		r["public-address"] = addr
+	}
+
+	if id, err := unit.AssignedMachineId(); err == nil {
+		// TODO(dfc) we could make this nicer, ie machine/0
+		r["machine"] = id
+	}
+
+	processVersion(r, unit)
+	return r, nil
+}
+
+type versioned interface {
+	AgentVersion() (version.Number, error)
+	ProposedAgentVersion() (version.Number, error)
+}
+
+func processVersion(r map[string]interface{}, v versioned) {
+	if v, err := v.AgentVersion(); err == nil {
+		r["agent-version"] = v.String()
+	}
+
+	if v, err := v.ProposedAgentVersion(); err == nil {
+		r["proposed-agent-version"] = v.String()
+	}
+}
+
+type agent interface {
+	AgentAlive() (bool, error)
+}
+
+func processAgentStatus(r map[string]interface{}, a agent) {
+	if alive, err := a.AgentAlive(); err == nil && alive {
+		r["agent-state"] = "running"
+	}
 }
 
 // jsonify converts the keys of the machines map into their string
@@ -213,10 +254,19 @@
 func jsonify(r map[string]interface{}) map[string]map[string]interface{} {
 	m := map[string]map[string]interface{}{
 		"services": r["services"].(map[string]interface{}),
-		"machines": make(map[string]interface{}),
+		"machines": m(),
 	}
 	for k, v := range r["machines"].(map[int]interface{}) {
 		m["machines"][strconv.Itoa(k)] = v
 	}
 	return m
 }
+
+func m() map[string]interface{} { return make(map[string]interface{}) }
+
+func chkError(m map[string]interface{}, err error) map[string]interface{} {
+	if err != nil {
+		return map[string]interface{}{"status-error": err.Error()}
+	}
+	return m
+}

=== modified file 'cmd/juju/status_test.go'
--- cmd/juju/status_test.go	2012-08-02 22:54:18 +0000
+++ cmd/juju/status_test.go	2012-08-02 22:54:18 +0000
@@ -181,6 +181,102 @@
 			},
 		},
 	},
+	{
+		"add two more machines for units",
+		func(st *state.State, conn *juju.Conn, c *C) {
+			for i := 1; i < 3; i++ {
+				m, err := st.AddMachine()
+				c.Assert(err, IsNil)
+				c.Assert(m.Id(), Equals, i)
+				inst, err := conn.Environ.StartInstance(m.Id(), nil)
+				c.Assert(err, IsNil)
+				err = m.SetInstanceId(inst.Id())
+				c.Assert(err, IsNil)
+			}
+		},
+		map[string]interface{}{
+			"machines": map[int]interface{}{
+				0: map[string]interface{}{
+					"dns-name":               "palermo-0.dns",
+					"instance-id":            "palermo-0",
+					"agent-version":          "1.2.3",
+					"proposed-agent-version": "2.0.3",
+				},
+				1: map[string]interface{}{
+					"dns-name":    "palermo-1.dns",
+					"instance-id": "palermo-1",
+				},
+				2: map[string]interface{}{
+					"dns-name":    "palermo-2.dns",
+					"instance-id": "palermo-2",
+				},
+			},
+                        "services": map[string]interface{}{
+                                "dummy-service": map[string]interface{}{
+                                        "charm":   "dummy",
+                                        "exposed": false,
+                                },
+                                "exposed-service": map[string]interface{}{
+                                        "charm":   "dummy",
+                                        "exposed": true,
+                                },
+                        },
+		},
+	},
+	{
+		"add units for services",
+		func(st *state.State, conn *juju.Conn, c *C) {
+			for i, n := range []string{"dummy-service", "exposed-service"} {
+				s, err := st.Service(n)
+				c.Assert(err, IsNil)
+				u, err := s.AddUnit()
+				c.Assert(err, IsNil)
+				m, err := st.Machine(i + 1)
+				c.Assert(err, IsNil)
+				err = u.AssignToMachine(m)
+				c.Assert(err, IsNil)
+			}
+		},
+		map[string]interface{}{
+			"machines": map[int]interface{}{
+				0: map[string]interface{}{
+					"dns-name":               "palermo-0.dns",
+					"instance-id":            "palermo-0",
+					"agent-version":          "1.2.3",
+					"proposed-agent-version": "2.0.3",
+				},
+				1: map[string]interface{}{
+					"dns-name":    "palermo-1.dns",
+					"instance-id": "palermo-1",
+				},
+				2: map[string]interface{}{
+					"dns-name":    "palermo-2.dns",
+					"instance-id": "palermo-2",
+				},
+			},
+                        "services": map[string]interface{}{
+                                "exposed-service": map[string]interface{}{
+                                        "exposed": true,
+                                        "units": map[string]interface{}{
+                                                "exposed-service/0": map[string]interface{}{
+                                                        "machine": 2,
+                                                },
+                                        },
+                                        "charm": "dummy",
+                                },
+                                "dummy-service": map[string]interface{}{
+                                        "charm":   "dummy",
+                                        "exposed": false,
+                                        "units": map[string]interface{}{
+                                                "dummy-service/0": map[string]interface{}{
+                                                        "machine": 1,
+                                                },
+                                        },
+                                },
+                        },
+		},
+	},
+
 	// TODO(dfc) test failing components by destructively mutating zk under the hood
 }