Merge lp:~gz/juju-core/status_api_1.16_compat into lp:~go-bot/juju-core/trunk

[Martin Packman]

Restore juju status compatibility with 1.16

A stub 'Status' api call was included in 1.16 though not used
in our codebase. It does break the fallback logic to switch
and operate on state directly when talking to a 1.16 state
server however. This branch reintroduces the old call with
the existing name, and uses 'FullStatus' for actual status
api.

https://codereview.appspot.com/66590043/

R=dimitern

Restore juju status compatibility with 1.16

A stub 'Status' api call was included in 1.16 though not used
in our codebase. It does break the fallback logic to switch
and operate on state directly when talking to a 1.16 state
server however. This branch reintroduces the old call with
the existing name, and uses 'FullStatus' for actual status
api.

https://codereview.appspot.com/66590043/

[Martin Packman (gz)]

Reviewers: mp+207482_code.launchpad.net,

Message:
Please take a look.

Description:
Restore juju status compatibility with 1.16

A stub 'Status' api call was included in 1.16 though not used
in our codebase. It does break the fallback logic to switch
and operate on state directly when talking to a 1.16 state
server however. This branch reintroduces the old call with
the existing name, and uses 'FullStatus' for actual status
api.

I want to add a test for the legacy Status method added in
state/apiserver/client/status.go just to exercise that
code. Where should it go?

https://code.launchpad.net/~gz/juju-core/status_api_1.16_compat/+merge/207482

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/66590043/

Affected files (+41, -2 lines):
   A [revision details]
   M state/api/client.go
   M state/apiserver/client/status.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140220084222-vp7yz0ga1ligfdkq
+New revision: <email address hidden>

Index: state/api/client.go
=== modified file 'state/api/client.go'
--- state/api/client.go 2014-01-30 02:20:09 +0000
+++ state/api/client.go 2014-02-20 15:56:17 +0000
@@ -77,7 +77,26 @@
  func (c *Client) Status(patterns []string) (*Status, error) {
   var s Status
   p := params.StatusParams{Patterns: patterns}
- if err := c.st.Call("Client", "", "Status", p, &s); err != nil {
+ if err := c.st.Call("Client", "", "FullStatus", p, &s); err != nil {
+ return nil, err
+ }
+ return &s, nil
+}
+
+// LegacyMachineStatus holds just the instance-id of a machine.
+type LegacyMachineStatus struct {
+ InstanceId string
+}
+
+// LegacyStatus holds minimal information on the status of a juju
environment.
+type LegacyStatus struct {
+ Machines map[string]LegacyMachineStatus
+}
+
+// LegacyStatus is a stub version of Status that 1.16 introduced.
+func (c *Client) LegacyStatus() (*LegacyStatus, error) {
+ var s LegacyStatus
+ if err := c.st.Call("Client", "", "Status", nil, &s); err != nil {
    return nil, err
   }
   return &s, nil

Index: state/apiserver/client/status.go
=== modified file 'state/apiserver/client/status.go'
--- state/apiserver/client/status.go 2013-12-19 18:13:24 +0000
+++ state/apiserver/client/status.go 2014-02-20 15:56:17 +0000
@@ -10,7 +10,8 @@
   "launchpad.net/juju-core/state/statecmd"
  )

-func (c *Client) Status(args params.StatusParams) (api.Status, error) {
+// FullStatus gives the information needed for juju status over the api
+func (c *Client) FullStatus(args params.StatusParams) (api.Status, error) {
   conn, err := juju.NewConnFromState(c.api.state)
   if err != nil {
    return api.Status{}, err
@@ -19,3 +20,20 @@
   status, err := statecmd.Status(conn, args.Patterns)
   return *status, err
  }
+
+// Status is a stub version of FullStatus that was introduced in 1.16
+func (c *Client) Status() (api.LegacyStatus, error) {
+ var legacyStatus api.LegacyStatus
+ status, err := c.FullStatus(params.StatusParams{})
+ if err != nil {
+ return legacyStat...

[Dimiter Naydenov (dimitern)]

Looks like a good start.

Tests usually go in apiserver/client/, and in this case perhaps it
should be in status_test.go. Look how similar calls are tested. There
are also api/client tests to verify end-to-end integration.

https://codereview.appspot.com/66590043/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/66590043/diff/1/state/api/client.go#newcode86
state/api/client.go:86: // LegacyMachineStatus holds just the
instance-id of a machine.
// Remove this in 1.20 or something?
(in all three doc comments below)

https://codereview.appspot.com/66590043/diff/1/state/api/client.go#newcode98
state/api/client.go:98: var s LegacyStatus
s/s/result/ - it's more often called like this in other calls.

https://codereview.appspot.com/66590043/diff/1/state/apiserver/client/status.go
File state/apiserver/client/status.go (right):

https://codereview.appspot.com/66590043/diff/1/state/apiserver/client/status.go#newcode25
state/apiserver/client/status.go:25: func (c *Client) Status()
(api.LegacyStatus, error) {
Shouldn't we keep the params.StatusParams argument?

https://codereview.appspot.com/66590043/

[Martin Packman (gz)]

Thanks Dimiter!

https://codereview.appspot.com/66590043/diff/1/state/api/client.go
File state/api/client.go (right):

https://codereview.appspot.com/66590043/diff/1/state/api/client.go#newcode86
state/api/client.go:86: // LegacyMachineStatus holds just the
instance-id of a machine.
On 2014/02/20 16:14:01, dimitern wrote:
> // Remove this in 1.20 or something?
> (in all three doc comments below)

Good idea. Can we actually remove it in 1.20 though? Don't we need api
versioning to incompatibily change the api at all?

https://codereview.appspot.com/66590043/diff/1/state/api/client.go#newcode98
state/api/client.go:98: var s LegacyStatus
On 2014/02/20 16:14:01, dimitern wrote:
> s/s/result/ - it's more often called like this in other calls.

Heh, this is actually how it was originally written, but I agree naming
it result (here and above) is clearer.

https://codereview.appspot.com/66590043/diff/1/state/apiserver/client/status.go
File state/apiserver/client/status.go (right):

https://codereview.appspot.com/66590043/diff/1/state/apiserver/client/status.go#newcode25
state/apiserver/client/status.go:25: func (c *Client) Status()
(api.LegacyStatus, error) {
On 2014/02/20 16:14:01, dimitern wrote:
> Shouldn't we keep the params.StatusParams argument?

This is where a longer-range diff would be useful. This method is
reproducing the old 1.16 status api method exactly, which didn't take
arguments and didn't return anything of much use. However, I'm readding
it for compat reasons.

https://codereview.appspot.com/66590043/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/66590043/

[Dimiter Naydenov (dimitern)]

Better, thanks.

Now you only need to add some api/client tests I think and should be
good to land.

https://codereview.appspot.com/66590043/diff/20001/state/apiserver/client/status_test.go
File state/apiserver/client/status_test.go (right):

https://codereview.appspot.com/66590043/diff/20001/state/apiserver/client/status_test.go#newcode29
state/apiserver/client/status_test.go:29: // XXX: Have to test via the
client even though this is in apiserver,
This is fine and not unusual (other tests use the client as well), so
remove this comment :)

https://codereview.appspot.com/66590043/

[Martin Packman (gz)]

Please take a look.

https://codereview.appspot.com/66590043/

[Dimiter Naydenov (dimitern)]

On 2014/02/20 17:11:01, dimitern wrote:
> Better, thanks.

> Now you only need to add some api/client tests I think and should be
good to
> land.

Forget about this, sorry. I was thinking of the agent api client tests.
LGTM.

https://codereview.appspot.com/66590043/

=== modified file 'state/api/client.go'
--- state/api/client.go	2014-01-30 02:20:09 +0000
+++ state/api/client.go	2014-02-24 17:41:32 +0000
@@ -75,12 +75,32 @@
 
 // Status returns the status of the juju environment.
 func (c *Client) Status(patterns []string) (*Status, error) {
-	var s Status
+	var result Status
 	p := params.StatusParams{Patterns: patterns}
-	if err := c.st.Call("Client", "", "Status", p, &s); err != nil {
-		return nil, err
-	}
-	return &s, nil
+	if err := c.st.Call("Client", "", "FullStatus", p, &result); err != nil {
+		return nil, err
+	}
+	return &result, nil
+}
+
+// LegacyMachineStatus holds just the instance-id of a machine.
+type LegacyMachineStatus struct {
+	InstanceId string // Not type instance.Id just to match original api.
+}
+
+// LegacyStatus holds minimal information on the status of a juju environment.
+type LegacyStatus struct {
+	Machines map[string]LegacyMachineStatus
+}
+
+// LegacyStatus is a stub version of Status that 1.16 introduced. Should be
+// removed along with structs when api versioning makes it safe to do so.
+func (c *Client) LegacyStatus() (*LegacyStatus, error) {
+	var result LegacyStatus
+	if err := c.st.Call("Client", "", "Status", nil, &result); err != nil {
+		return nil, err
+	}
+	return &result, nil
 }
 
 // ServiceSet sets configuration options on a service.

=== modified file 'state/apiserver/client/status.go'
--- state/apiserver/client/status.go	2013-12-19 18:13:24 +0000
+++ state/apiserver/client/status.go	2014-02-24 17:41:32 +0000
@@ -10,7 +10,8 @@
 	"launchpad.net/juju-core/state/statecmd"
 )
 
-func (c *Client) Status(args params.StatusParams) (api.Status, error) {
+// FullStatus gives the information needed for juju status over the api
+func (c *Client) FullStatus(args params.StatusParams) (api.Status, error) {
 	conn, err := juju.NewConnFromState(c.api.state)
 	if err != nil {
 		return api.Status{}, err
@@ -19,3 +20,20 @@
 	status, err := statecmd.Status(conn, args.Patterns)
 	return *status, err
 }
+
+// Status is a stub version of FullStatus that was introduced in 1.16
+func (c *Client) Status() (api.LegacyStatus, error) {
+	var legacyStatus api.LegacyStatus
+	status, err := c.FullStatus(params.StatusParams{})
+	if err != nil {
+		return legacyStatus, err
+	}
+
+	legacyStatus.Machines = make(map[string]api.LegacyMachineStatus)
+	for machineName, machineStatus := range status.Machines {
+		legacyStatus.Machines[machineName] = api.LegacyMachineStatus{
+			InstanceId: string(machineStatus.InstanceId),
+		}
+	}
+	return legacyStatus, nil
+}

=== added file 'state/apiserver/client/status_test.go'
--- state/apiserver/client/status_test.go	1970-01-01 00:00:00 +0000
+++ state/apiserver/client/status_test.go	2014-02-24 17:41:32 +0000
@@ -0,0 +1,58 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package client_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/instance"
+	"launchpad.net/juju-core/state"
+)
+
+type statusSuite struct {
+	baseSuite
+}
+
+var _ = gc.Suite(&statusSuite{})
+
+func (s *statusSuite) addMachine(c *gc.C) *state.Machine {
+	machine, err := s.State.AddMachine("quantal", state.JobHostUnits)
+	c.Assert(err, gc.IsNil)
+	return machine
+}
+
+// Complete testing of status functionality happens elsewhere in the codebase,
+// these tests just sanity-check the api itself.
+
+func (s *statusSuite) TestFullStatus(c *gc.C) {
+	machine := s.addMachine(c)
+	client := s.APIState.Client()
+	status, err := client.Status(nil)
+	c.Assert(err, gc.IsNil)
+	c.Check(status.EnvironmentName, gc.Equals, "dummyenv")
+	c.Check(status.Services, gc.HasLen, 0)
+	c.Check(status.Machines, gc.HasLen, 1)
+	resultMachine, ok := status.Machines[machine.Id()]
+	if !ok {
+		c.Fatalf("Missing machine with id %q", machine.Id())
+	}
+	c.Check(resultMachine.Id, gc.Equals, machine.Id())
+	c.Check(resultMachine.Series, gc.Equals, machine.Series())
+}
+
+func (s *statusSuite) TestLegacyStatus(c *gc.C) {
+	machine := s.addMachine(c)
+	instanceId := "i-fakeinstance"
+	err := machine.SetProvisioned(instance.Id(instanceId), "fakenonce", nil)
+	c.Assert(err, gc.IsNil)
+	client := s.APIState.Client()
+	status, err := client.LegacyStatus()
+	c.Assert(err, gc.IsNil)
+	c.Check(status.Machines, gc.HasLen, 1)
+	resultMachine, ok := status.Machines[machine.Id()]
+	if !ok {
+		c.Fatalf("Missing machine with id %q", machine.Id())
+	}
+	c.Check(resultMachine.InstanceId, gc.Equals, instanceId)
+}