state/api/machineagent: new package

state/api/machineagent: new package

This provides the client side of state/apiserver/machine.AgentAPI.

https://code.launchpad.net/~rogpeppe/juju-core/324-machineagent-api-client-2/+merge/170541

Requires: https://code.launchpad.net/~rogpeppe/juju-core/323-machineagent-api-client/+merge/170400

(do not edit description out of merge proposal)

[rog, 2013-06-20 07:44:31]

Please take a look.

[wallyworld, 2013-06-21 01:23:43]

LGTM with the suggested fixes.

https://codereview.appspot.com/10437043/diff/2001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/10437043/diff/2001/juju/testing/conn.go#newcod...
juju/testing/conn.go:158: func (s *JujuConnSuite) OpenAPIAs(c *C, tag, password
string) *api.State {
Please add a method comment since it's an exported method and I'm not sure what
tag refers to. The "As" in the method name implies tag may be the identity being
used or something?

https://codereview.appspot.com/10437043/diff/2001/juju/testing/conn.go#newcod...
juju/testing/conn.go:162: info.Password = "password"
Why "password"? Shouldn't it be using the method param?

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
File state/api/machineagent/state_test.go (right):

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:65: c.Assert(err, ErrorMatches,
fmt.Sprintf("machine %s not found", s.machine.Id()))
Don't we have a common errors.NotFoundError? And a helper method IsNotFound?

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:77: 
Do we need to check that Life is still "alive" before the refresh?

[rog, 2013-06-21 09:36:16]

https://codereview.appspot.com/10437043/diff/2001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/10437043/diff/2001/juju/testing/conn.go#newcod...
juju/testing/conn.go:158: func (s *JujuConnSuite) OpenAPIAs(c *C, tag, password
string) *api.State {
On 2013/06/21 01:23:43, wallyworld wrote:
> Please add a method comment since it's an exported method and I'm not sure
what
> tag refers to. The "As" in the method name implies tag may be the identity
being
> used or something?

Done.

https://codereview.appspot.com/10437043/diff/2001/juju/testing/conn.go#newcod...
juju/testing/conn.go:162: info.Password = "password"
On 2013/06/21 01:23:43, wallyworld wrote:
> Why "password"? Shouldn't it be using the method param?

ahem, good catch!
done.

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
File state/api/machineagent/state_test.go (right):

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:65: c.Assert(err, ErrorMatches,
fmt.Sprintf("machine %s not found", s.machine.Id()))
On 2013/06/21 01:23:43, wallyworld wrote:
> Don't we have a common errors.NotFoundError? And a helper method IsNotFound?

API errors are currently not integrated with the errors package.
It should happen, one way or another, but it's not something
to do in this CL.

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:77: 
On 2013/06/21 01:23:43, wallyworld wrote:
> Do we need to check that Life is still "alive" before the refresh?

i think so - that's why we do it.

[fwereade, 2013-06-21 09:42:18]

LGTM, thanks

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
File state/api/machineagent/state.go (right):

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state.go:12: // State provides access to a machiner
worker's view of the state.
machine agent's view of state?

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
File state/api/machineagent/state_test.go (right):

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:65: c.Assert(err, ErrorMatches,
fmt.Sprintf("machine %s not found", s.machine.Id()))
On 2013/06/21 01:23:43, wallyworld wrote:
> Don't we have a common errors.NotFoundError? And a helper method IsNotFound?

Yeah, I'd naturally expect us to translate these back... fine to leave it out of
this CL though I guess.

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:77: 
On 2013/06/21 01:23:43, wallyworld wrote:
> Do we need to check that Life is still "alive" before the refresh?

Hmm. Can't hurt to be explicit, I guess; that is a behaviour change.

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:77: 
On 2013/06/21 09:36:16, rog wrote:
> On 2013/06/21 01:23:43, wallyworld wrote:
> > Do we need to check that Life is still "alive" before the refresh?
> 
> i think so - that's why we do it.

before refresh, not before destroy

[rog, 2013-06-21 09:54:59]

Please take a look.

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
File state/api/machineagent/state.go (right):

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state.go:12: // State provides access to a machiner
worker's view of the state.
On 2013/06/21 09:42:18, fwereade wrote:
> machine agent's view of state?

ah yes, thanks.

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
File state/api/machineagent/state_test.go (right):

https://codereview.appspot.com/10437043/diff/2001/state/api/machineagent/stat...
state/api/machineagent/state_test.go:77: 
On 2013/06/21 09:42:18, fwereade wrote:
> On 2013/06/21 09:36:16, rog wrote:
> > On 2013/06/21 01:23:43, wallyworld wrote:
> > > Do we need to check that Life is still "alive" before the refresh?
> > 
> > i think so - that's why we do it.
> 
> before refresh, not before destroy

I have no idea how calling Destroy on a state.Machine could cause an api.Machine
life status to change spontaneously. They're totally separate objects that don't
change asynchronously. I'd prefer to stick to testing moderately plausible code
paths.