state/api/agent: new package

state/api/agent: new package

This provides functionality for the machine agent and unit agent.
The old MachineAgent API entry point is now deprecated;
the client entry point has been removed but the server
side remains around until we can remove it (for backward
compatibility with 1.12).

https://code.launchpad.net/~rogpeppe/juju-core/352-agent-api/+merge/177440

(do not edit description out of merge proposal)

[rog, 2013-07-29 17:44:57]

Please take a look.

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent.go
File state/apiserver/agent/agent.go (right):

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent.go...
state/apiserver/agent/agent.go:6: package agent
this package is essentially just a copy of the machine.AgentAPI code.

[dimitern, 2013-07-29 17:57:55]

Looking great, apart from some minor quibbles bellow

https://codereview.appspot.com/12038045/diff/1/state/api/agent/state_test.go
File state/api/agent/state_test.go (right):

https://codereview.appspot.com/12038045/diff/1/state/api/agent/state_test.go#...
state/api/agent/state_test.go:11: "launchpad.net/juju-core/errors"

Blank line after gc ...

https://codereview.appspot.com/12038045/diff/1/state/api/agent/state_test.go#...
state/api/agent/state_test.go:103: type unitSuite struct {

I'd prefer having a separate machine_test.go and unit_test.go, rather than
mixing them in the same file.

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent_te...
File state/apiserver/agent/agent_test.go (right):

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent_te...
state/apiserver/agent/agent_test.go:4: gc "launchpad.net/gocheck"

Move gc ... in a separate line, after stdlib packages, surrounded by blank
lines.

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent_te...
state/apiserver/agent/agent_test.go:51: api, err := agent.NewAPI(s.State,
s.authorizer)

Why not:
s.agent, err = agent.NewAPI(...)
?

https://codereview.appspot.com/12038045/diff/1/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/12038045/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:48: logger.Infof("upgrader upgraded from %v to %v
(%q)", e.OldTools.Version, tools.Version, tools.URL)

Like I said in the previous review, "upgrader" is not needed here, because the
logger is already defined for "juju.worker.upgrader".

[rog, 2013-07-30 07:45:33]

Please take a look.

https://codereview.appspot.com/12038045/diff/1/state/api/agent/state_test.go
File state/api/agent/state_test.go (right):

https://codereview.appspot.com/12038045/diff/1/state/api/agent/state_test.go#...
state/api/agent/state_test.go:11: "launchpad.net/juju-core/errors"
On 2013/07/29 17:57:56, dimitern wrote:
> 
> Blank line after gc ...

Done.

https://codereview.appspot.com/12038045/diff/1/state/api/agent/state_test.go#...
state/api/agent/state_test.go:103: type unitSuite struct {
On 2013/07/29 17:57:56, dimitern wrote:
> 
> I'd prefer having a separate machine_test.go and unit_test.go, rather than
> mixing them in the same file.

Done.

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent_te...
File state/apiserver/agent/agent_test.go (right):

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent_te...
state/apiserver/agent/agent_test.go:4: gc "launchpad.net/gocheck"
On 2013/07/29 17:57:56, dimitern wrote:
> 
> Move gc ... in a separate line, after stdlib packages, surrounded by blank
> lines.

Done.

https://codereview.appspot.com/12038045/diff/1/state/apiserver/agent/agent_te...
state/apiserver/agent/agent_test.go:51: api, err := agent.NewAPI(s.State,
s.authorizer)
On 2013/07/29 17:57:56, dimitern wrote:
> 
> Why not:
> s.agent, err = agent.NewAPI(...)
> ?

Done.

[mue, 2013-07-30 07:59:46]

LGTM