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.