state/api: Split client Machiner into subpackage

state/api: Split client Machiner into subpackage

Following up on https://codereview.appspot.com/10044043/
this does the same for the client-side: splits
machiner facade into its own pacakge.

https://code.launchpad.net/~dimitern/juju-core/056-api-machiner-client-subpackage/+merge/167489

Requires: https://code.launchpad.net/~dimitern/juju-core/055-api-machiner-subpackages/+merge/167486

(do not edit description out of merge proposal)

[dimitern, 2013-06-05 09:44:12]

Please take a look.

[fwereade, 2013-06-05 09:49:08]

LGTM, although I have same uncertainty about "apimachiner" vs "machiner" as
expressed in the prereq.

[dimitern, 2013-06-05 10:20:21]

On 2013/06/05 09:49:08, fwereade wrote:
> LGTM, although I have same uncertainty about "apimachiner" vs "machiner" as
> expressed in the prereq.

Changing them as in the prereq.

[dimitern, 2013-06-05 10:56:02]

Please take a look.

[rog, 2013-06-05 16:52:16]

looks reasonable, with a few comments and suggestions below.

https://codereview.appspot.com/10026044/diff/5001/state/api/common/interfaces.go
File state/api/common/interfaces.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/common/interfaces...
state/api/common/interfaces.go:6: // Caller interface is implemented by the
client-facing State object.
// Caller is implemented by the client-facing State object.

https://codereview.appspot.com/10026044/diff/5001/state/api/common/interfaces...
state/api/common/interfaces.go:8: Call(objType, id, request string, params,
response interface{}) error
// Call makes a call to the API server with the given
// object type, id, request and parameters. The response
// is filled in with the call's result if the call
// is successful.

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine.go
File state/api/machiner/machine.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine....
state/api/machiner/machine.go:4: package machiner
i'm not convinced by the need for this package.

i think we could use a single client api package for all the various views of
the state provided by the api server. it would mean that some calls would yield
"no such method" when they were not provided by a particular view, but it would
save us lots of code with not many other down sides that i can see.

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine....
state/api/machiner/machine.go:8: // Machine provides access to state.Machine
methods through
// Machine represents a juju machine as seen
// by a machiner worker.

?

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine....
state/api/machiner/machine.go:16: // SetStatus changes the status of the
machine.
s/changes/sets/

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machiner.go
File state/api/machiner/machiner.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machiner...
state/api/machiner/machiner.go:14: stcaller common.Caller
s/stcaller/caller/ ?

https://codereview.appspot.com/10026044/diff/5001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/state.go#newcode29
state/api/state.go:29: func (st *State) Machiner(version string)
(*machiner.Machiner, error) {
what's the "version" string here?

i didn't think we had yet agreed on how versioning was to be done. in
particular, i'm not sure that it's right that the version be an argument to the
State.Machiner method, which is already tied to a particular version by virtue
of the calls that it implements and the types that it passes to them.

please can we lose the argument to this function for the time being and
hard-code "" in the call below.

https://codereview.appspot.com/10026044/diff/5001/state/api/watcher.go
File state/api/watcher.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/watcher.go#newcode21
state/api/watcher.go:21: func newAllWatcher(client *Client, id *string)
*AllWatcher {
why is the id a pointer here?

[dimitern, 2013-06-06 09:57:04]

Please take a look.

https://codereview.appspot.com/10026044/diff/5001/state/api/common/interfaces.go
File state/api/common/interfaces.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/common/interfaces...
state/api/common/interfaces.go:6: // Caller interface is implemented by the
client-facing State object.
On 2013/06/05 16:52:17, rog wrote:
> // Caller is implemented by the client-facing State object.

Done.

https://codereview.appspot.com/10026044/diff/5001/state/api/common/interfaces...
state/api/common/interfaces.go:8: Call(objType, id, request string, params,
response interface{}) error
On 2013/06/05 16:52:17, rog wrote:
> // Call makes a call to the API server with the given
> // object type, id, request and parameters. The response
> // is filled in with the call's result if the call
> // is successful.

Done.

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine.go
File state/api/machiner/machine.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine....
state/api/machiner/machine.go:4: package machiner
On 2013/06/05 16:52:17, rog wrote:
> i'm not convinced by the need for this package.
> 
> i think we could use a single client api package for all the various views of
> the state provided by the api server. it would mean that some calls would
yield
> "no such method" when they were not provided by a particular view, but it
would
> save us lots of code with not many other down sides that i can see.

I think we agreed to make smaller self-contained packages for easier
maintainability. In addition it allows us to namespace stuff needed by the
facades and use machiner.Machine rather than api.MachinerMachine, because we'll
need the same for the provisioner, firewaller, etc.

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine....
state/api/machiner/machine.go:8: // Machine provides access to state.Machine
methods through
On 2013/06/05 16:52:17, rog wrote:
> // Machine represents a juju machine as seen
> // by a machiner worker.
> 
> ?

Done.

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machine....
state/api/machiner/machine.go:16: // SetStatus changes the status of the
machine.
On 2013/06/05 16:52:17, rog wrote:
> s/changes/sets/

Done.

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machiner.go
File state/api/machiner/machiner.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/machiner/machiner...
state/api/machiner/machiner.go:14: stcaller common.Caller
On 2013/06/05 16:52:17, rog wrote:
> s/stcaller/caller/ ?

Done.

https://codereview.appspot.com/10026044/diff/5001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/state.go#newcode29
state/api/state.go:29: func (st *State) Machiner(version string)
(*machiner.Machiner, error) {
On 2013/06/05 16:52:17, rog wrote:
> what's the "version" string here?
> 
> i didn't think we had yet agreed on how versioning was to be done. in
> particular, i'm not sure that it's right that the version be an argument to
the
> State.Machiner method, which is already tied to a particular version by virtue
> of the calls that it implements and the types that it passes to them.
> 
> please can we lose the argument to this function for the time being and
> hard-code "" in the call below.

We had a discussion we can use these arguments to implement versioning possibly.
This does not do any actual steps towards supporting that, except the argument
which might be used in the future. Also this is required for the
TestMachinerFailsWithNotEmptyVersion, which is generally the way to check we
always pass an empty string.

https://codereview.appspot.com/10026044/diff/5001/state/api/watcher.go
File state/api/watcher.go (right):

https://codereview.appspot.com/10026044/diff/5001/state/api/watcher.go#newcode21
state/api/watcher.go:21: func newAllWatcher(client *Client, id *string)
*AllWatcher {
On 2013/06/05 16:52:17, rog wrote:
> why is the id a pointer here?

I haven't touched this code.

[dimitern, 2013-06-06 17:19:21]

Please take a look.

[dimitern, 2013-06-06 17:21:07]

Please take a look.

[rog, 2013-06-06 17:29:49]

LGTM with a couple of trivials below.

https://codereview.appspot.com/10026044/diff/24001/state/api/machiner/machine...
File state/api/machiner/machiner_test.go (right):

https://codereview.appspot.com/10026044/diff/24001/state/api/machiner/machine...
state/api/machiner/machiner_test.go:85: func (s *machinerSuite)
TestMachinerFailsWithNotEmptyId(c *C) {
s/NotEmpty/NonEmpty/

https://codereview.appspot.com/10026044/diff/24001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/10026044/diff/24001/state/api/state.go#newcode30
state/api/state.go:30: func (st *State) Machiner(id string) (*machiner.Machiner,
error) {
i think this argument should go, as discussed.
we can add it later if/when it becomes meaningful.

[dimitern, 2013-06-06 17:56:11]

*** Submitted:

state/api: Split client Machiner into subpackage

Following up on https://codereview.appspot.com/10044043/
this does the same for the client-side: splits
machiner facade into its own pacakge.

R=fwereade, rog
CC=
https://codereview.appspot.com/10026044

https://codereview.appspot.com/10026044/diff/24001/state/api/machiner/machine...
File state/api/machiner/machiner_test.go (right):

https://codereview.appspot.com/10026044/diff/24001/state/api/machiner/machine...
state/api/machiner/machiner_test.go:85: func (s *machinerSuite)
TestMachinerFailsWithNotEmptyId(c *C) {
On 2013/06/06 17:29:50, rog wrote:
> s/NotEmpty/NonEmpty/

Removed this test, as discussed online and added a TODO in apiclient.Call to add
tests for all such cases in one place.

https://codereview.appspot.com/10026044/diff/24001/state/api/state.go
File state/api/state.go (right):

https://codereview.appspot.com/10026044/diff/24001/state/api/state.go#newcode30
state/api/state.go:30: func (st *State) Machiner(id string) (*machiner.Machiner,
error) {
On 2013/06/06 17:29:50, rog wrote:
> i think this argument should go, as discussed.
> we can add it later if/when it becomes meaningful.

Done.