state: AddMachine revamp continues

state: AddMachine revamp continues

We bring the exported State methods closer to the underlying
implementation.

The old interface is still visible in the API, which we can't
easily change for backward compatibility reasons.

As a drive-by fix, we also remove state.Machine.TxnRevno because it's
unnecessary.

https://code.launchpad.net/~rogpeppe/juju-core/468-state-addmachines/+merge/197934

(do not edit description out of merge proposal)

[rog, 2013-12-05 18:46:07]

Please take a look.

[dimitern, 2013-12-06 13:13:37]

Looks great so far, just a bunch of mostly trivials. TxnRevNo removal should be
part of the CL description.

https://codereview.appspot.com/37850043/diff/40001/juju/conn.go
File juju/conn.go (left):

https://codereview.appspot.com/37850043/diff/40001/juju/conn.go#oldcode366
juju/conn.go:366: // See https://bugs.launchpad.net/juju-core/+bug/1252799
Link this branch to bug 1252799 please.

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcode68
state/addmachine.go:68: // AddMachineInsideNewMachine creates a new machine
within a container
This feels like Inception :)

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcode74
state/addmachine.go:74: return nil, fmt.Errorf("cannot add a new container: %v",
err)
s/container/machine/ ?

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcode84
state/addmachine.go:84: return nil, fmt.Errorf("cannot add a new container: %v",
err)
ditto?

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcod...
state/addmachine.go:102: // AddOne machine adds a new machine configured
according to the
s/AddOne/AddOneMachine/

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcod...
state/addmachine.go:104: func (st *State) AddOneMachine(template
MachineTemplate) (*Machine, error) {
Perhaps call this AddMachineWithTemplate? AddOneMachine is confusing given the
above call which seems to do the same, judging by the names.

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcod...
state/addmachine.go:172: return MachineTemplate{}, fmt.Errorf("cannot inject a
machine without a nonce")
s/inject/add/ ? for consistency

https://codereview.appspot.com/37850043/diff/40001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/api/params/params.go...
state/api/params/params.go:83: // series, constraints and jobs as the new
container.
s/the same series, constraints and jobs as the new container/the given series,
constraints, and jobs/ - I find that easier to parse.

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1635: func (s *clientSuite)
TestClientInjectMachinesDefaultSeries(c *gc.C) {
why are these two tests removed?

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1580: c.Assert(machines[2].Error,
gc.NotNil)
not even sure we need this when we're using ErrorMatches just below.

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1582: c.Assert(machines[2].Error,
gc.NotNil)
d

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1615: machines, err :=
s.APIState.Client().AddMachines(apiParams)
I thought we're not changing the client API for compatibility sake? If we're
renaming InjectMachines to AddMachines, we need to have the old method there and
also a test that both the new and the old one do the same (with a comment about
testing backward compatibility). Unless I'm not getting something right..

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1650: c.Assert(machineResult.Error,
gc.NotNil)
again, not sure we need NotNil with ErrorMatches after that - the latter should
be enough.

https://codereview.appspot.com/37850043/diff/40001/state/cleanup_test.go
File state/cleanup_test.go (left):

https://codereview.appspot.com/37850043/diff/40001/state/cleanup_test.go#oldc...
state/cleanup_test.go:156: c.Assert(err, gc.IsNil)
why delete this?

https://codereview.appspot.com/37850043/diff/40001/state/machine.go
File state/machine.go (left):

https://codereview.appspot.com/37850043/diff/40001/state/machine.go#oldcode86
state/machine.go:86: TxnRevno      int64        `bson:"txn-revno"`
I'm ok with landing txnrevno removal with this CL, but please update the
description to mention it.

https://codereview.appspot.com/37850043/diff/40001/state/machine_test.go
File state/machine_test.go (left):

https://codereview.appspot.com/37850043/diff/40001/state/machine_test.go#oldc...
state/machine_test.go:114: c.Assert(err, gc.IsNil)
why remove this assert?

https://codereview.appspot.com/37850043/diff/40001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/watcher.go#newcode1116
state/watcher.go:1116: func getTxnRevno(coll *mgo.Collection, key string)
(int64, error) {
a little doc comment explaining why we need this would be helpful.

[rog, 2013-12-19 21:03:25]

Please take a look.

https://codereview.appspot.com/37850043/diff/40001/juju/conn.go
File juju/conn.go (left):

https://codereview.appspot.com/37850043/diff/40001/juju/conn.go#oldcode366
juju/conn.go:366: // See https://bugs.launchpad.net/juju-core/+bug/1252799
On 2013/12/06 13:13:37, dimitern wrote:
> Link this branch to bug 1252799 please.

Done.

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcode74
state/addmachine.go:74: return nil, fmt.Errorf("cannot add a new container: %v",
err)
On 2013/12/06 13:13:37, dimitern wrote:
> s/container/machine/ ?

i never know when to refer to machines as containers...
we do different things in different places.

done.

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcod...
state/addmachine.go:104: func (st *State) AddOneMachine(template
MachineTemplate) (*Machine, error) {
On 2013/12/06 13:13:37, dimitern wrote:
> Perhaps call this AddMachineWithTemplate? AddOneMachine is confusing given the
> above call which seems to do the same, judging by the names.

I'm planning to move AddMachine out of state entirely.
Then we can rename AddOneMachine to AddMachine.

"AddMachineWithTemplate" is also confusing because the other AddMachine calls
have templates too.

https://codereview.appspot.com/37850043/diff/40001/state/addmachine.go#newcod...
state/addmachine.go:172: return MachineTemplate{}, fmt.Errorf("cannot inject a
machine without a nonce")
On 2013/12/06 13:13:37, dimitern wrote:
> s/inject/add/ ? for consistency

done, slightly differently.

https://codereview.appspot.com/37850043/diff/40001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/api/params/params.go...
state/api/params/params.go:83: // series, constraints and jobs as the new
container.
On 2013/12/06 13:13:37, dimitern wrote:
> s/the same series, constraints and jobs as the new container/the given series,
> constraints, and jobs/ - I find that easier to parse.

Done.

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1635: func (s *clientSuite)
TestClientInjectMachinesDefaultSeries(c *gc.C) {
On 2013/12/06 13:13:37, dimitern wrote:
> why are these two tests removed?

Because TestClientAddMachinesDefaultSeries and TestClientAddMachinesWithSeries
already exist and duplicate the functionality.

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1580: c.Assert(machines[2].Error,
gc.NotNil)
On 2013/12/06 13:13:37, dimitern wrote:
> not even sure we need this when we're using ErrorMatches just below.

We do.

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1582: c.Assert(machines[2].Error,
gc.NotNil)
On 2013/12/06 13:13:37, dimitern wrote:
> d

ditto

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1615: machines, err :=
s.APIState.Client().AddMachines(apiParams)
On 2013/12/06 13:13:37, dimitern wrote:
> I thought we're not changing the client API for compatibility sake? If we're
> renaming InjectMachines to AddMachines, we need to have the old method there
and
> also a test that both the new and the old one do the same (with a comment
about
> testing backward compatibility). Unless I'm not getting something right..

I've added a test that InjectMachines still exists.
Given that it's a single line of code that calls AddMachines
and that I changed all the existing InjectMachine tests to
call AddMachines instead, I think that should be ok.

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1650: c.Assert(machineResult.Error,
gc.NotNil)
On 2013/12/06 13:13:37, dimitern wrote:
> again, not sure we need NotNil with ErrorMatches after that - the latter
should
> be enough.

Unfortunately not.
When the error is nil, ErrorMatches panics because it tries to call
params.Error.Error on a nil *params.Error.

https://codereview.appspot.com/37850043/diff/40001/state/cleanup_test.go
File state/cleanup_test.go (left):

https://codereview.appspot.com/37850043/diff/40001/state/cleanup_test.go#oldc...
state/cleanup_test.go:156: c.Assert(err, gc.IsNil)
On 2013/12/06 13:13:37, dimitern wrote:
> why delete this?

Done.

https://codereview.appspot.com/37850043/diff/40001/state/machine_test.go
File state/machine_test.go (left):

https://codereview.appspot.com/37850043/diff/40001/state/machine_test.go#oldc...
state/machine_test.go:114: c.Assert(err, gc.IsNil)
On 2013/12/06 13:13:37, dimitern wrote:
> why remove this assert?

editing error i think. done.

https://codereview.appspot.com/37850043/diff/40001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/watcher.go#newcode1116
state/watcher.go:1116: func getTxnRevno(coll *mgo.Collection, key string)
(int64, error) {
On 2013/12/06 13:13:37, dimitern wrote:
> a little doc comment explaining why we need this would be helpful.

Done.

[dimitern, 2013-12-19 21:09:29]

LGTM, thank you.

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/37850043/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1650: c.Assert(machineResult.Error,
gc.NotNil)
On 2013/12/19 21:03:26, rog wrote:
> On 2013/12/06 13:13:37, dimitern wrote:
> > again, not sure we need NotNil with ErrorMatches after that - the latter
> should
> > be enough.
> 
> Unfortunately not.
> When the error is nil, ErrorMatches panics because it tries to call
> params.Error.Error on a nil *params.Error.

Good to know! Thanks.

[rog, 2013-12-19 21:11:29]

Please take a look.

[rog, 2013-12-20 12:17:12]

Please take a look.