Continued development of the machine state.

State methods and tests for the machine state.

https://code.launchpad.net/~themue/juju/go-state-continued-machine/+merge/93982

(do not edit description out of merge proposal)

[TheMue, 2012-02-21 12:48:58]

Please take a look.

[fwereade, 2012-02-21 13:21:38]

Generally LGTM, couple of comments

https://codereview.appspot.com/5690051/diff/1/state/export_test.go
File state/export_test.go (right):

https://codereview.appspot.com/5690051/diff/1/state/export_test.go#newcode22
state/export_test.go:22: func ZkSetUpEnvironment(t *testing.T)
(*zookeeper.Server, string) {
I'm glad we're sharing this for the whole package now; how feasible would it be
to move it somewhere else so I can reuse it for (say) the state/presence tests?

It's not a *lot* of duplication yet, but I suspect we'll end up with more than a
few tests which happen to use a local ZK...

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

https://codereview.appspot.com/5690051/diff/1/state/state_test.go#newcode27
state/state_test.go:27: return filepath.Join("..", "charm", "testrepo",
"series", name)
(1) I think I'd prefer it if we copied the test charm out of the bzr tree before
we let test code play with it.

(2) Regardless, is there any way we can get a path without using ".."?

[TheMue, 2012-02-23 10:24:37]

Please take a look.

[TheMue, 2012-02-27 10:30:52]

Please take a look.

[TheMue, 2012-02-28 09:15:57]

Please take a look.

[fwereade, 2012-03-09 11:14:50]

https://codereview.appspot.com/5690051/diff/13001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5690051/diff/13001/state/state.go#newcode43
state/state.go:43: key := fmt.Sprintf("machine-%010d", id)
It'd be quite nice to move this out into its own little internalMachineId func

https://codereview.appspot.com/5690051/diff/13001/state/state.go#newcode44
state/state.go:44: mustDelete := false
Maybe call this shouldDelete instead? As stated below it's not actually a very
big deal if we accidentally keep an unreferenced machine node lying around.

https://codereview.appspot.com/5690051/diff/13001/state/state.go#newcode48
state/state.go:48: // with concurrency easier.
I'm not sure this is actually sensible: the only non-test code that calls this
(in Python) immediately turns round and raises a MachineStateNotFound if the
state wasn't actually deleted.

My personal inclination would be to integrate the python
juju.control.terminate_machine error checks (for trying-to-delete-machine-0, and
for didn't-actually-delete-machine) into this method: feels like it'd keep the
relevant code more coherent.

[TheMue, 2012-03-13 15:38:41]

Please take a look.

[TheMue, 2012-03-13 15:42:26]

PTAL

https://codereview.appspot.com/5690051/diff/13001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5690051/diff/13001/state/state.go#newcode43
state/state.go:43: key := fmt.Sprintf("machine-%010d", id)
On 2012/03/09 11:14:50, fwereade wrote:
> It'd be quite nice to move this out into its own little internalMachineId func

Done. It's machineKey(id) as counterpart to machineId(key) in machine.go.

https://codereview.appspot.com/5690051/diff/13001/state/state.go#newcode44
state/state.go:44: mustDelete := false
On 2012/03/09 11:14:50, fwereade wrote:
> Maybe call this shouldDelete instead? As stated below it's not actually a very
> big deal if we accidentally keep an unreferenced machine node lying around.

Done.

[fwereade, 2012-03-13 17:02:43]

LGTM modulo quibbles

https://codereview.appspot.com/5690051/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode65
state/state.go:65: key := fmt.Sprintf("machine-%010d", id)
shouldn't this be using machineKey?

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode149
state/state_test.go:149: machineTwo, err := s.st.AddMachine()
I think I'd prefer to call these machine0 and machine1 for clarity's sake...

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode161
state/state_test.go:161: machineOne, err := s.st.AddMachine()
ditto

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode184
state/state_test.go:184: c.Assert(machineOne.Id(), Equals, machineTwo.Id())
YMMV, but I think the following might be a little clearer:

machine, err := s.st.AddMachine()
c.Assert(err, IsNil)
expectId := machine.Id()
machine, err = s.st.Machine(expectId)
c.Assert(err, IsNil)
c.Assert(machine.Id(), Equals, expectId)

[TheMue, 2012-03-13 18:22:53]

Please take a look.

[niemeyer, 2012-03-13 19:23:14]

https://codereview.appspot.com/5690051/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode46
state/state.go:46: return fmt.Errorf("machine %d does not exist", id)
return fmt.Errorf("machine not found")

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode53
state/state.go:53: return fmt.Errorf("machine %d in use", id)
return fmt.Errorf("machine has units")

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode58
state/state.go:58: return err
return fmt.Errorf("can't remove machine %d: %v", id, err)

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode145
state/state_test.go:145: // Check that adding machines works correctly.
Sorry for nitpicking, but these comments aren't really adding any information
that is not present in the code. Comments are great when they are correct and
inform the reader of something that isn't otherwise trivially available. Several
of these one-liners are simply re-stating the method name:

    TestAddMachine == test that add machine works.

That said, the tests themselves look good here, thanks. The fact I'm nitpicking
rather worried about the tests themselves is great. :-)

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode160
state/state_test.go:160: // Check that removing a machine doesn't fail.
Same.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode179
state/state_test.go:179: // Check that reading a machine doesn't fail.
Same.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode188
state/state_test.go:188: // Check that reading a non-existent machine fails
nicely.
Same.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode199
state/state_test.go:199: // Check that reading all machines works correctly.
Same.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode218
state/state_test.go:218: // Check that adding services works correctly.
Same.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode247
state/state_test.go:247: // Check that removing the service works correctly.
Same.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode255
state/state_test.go:255: // Check that reading a non-existent service fails
nicely.
Same.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode261
state/state_test.go:261: // Check without existing services.
Same.

[TheMue, 2012-03-13 19:57:13]

Please take a look.

[TheMue, 2012-03-13 19:57:28]

PTAL

https://codereview.appspot.com/5690051/diff/19001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode46
state/state.go:46: return fmt.Errorf("machine %d does not exist", id)
On 2012/03/13 19:23:19, niemeyer wrote:
> return fmt.Errorf("machine not found")

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode53
state/state.go:53: return fmt.Errorf("machine %d in use", id)
On 2012/03/13 19:23:19, niemeyer wrote:
> return fmt.Errorf("machine has units")

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode58
state/state.go:58: return err
On 2012/03/13 19:23:19, niemeyer wrote:
> return fmt.Errorf("can't remove machine %d: %v", id, err)

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state.go#newcode65
state/state.go:65: key := fmt.Sprintf("machine-%010d", id)
On 2012/03/13 17:02:43, fwereade wrote:
> shouldn't this be using machineKey?

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode145
state/state_test.go:145: // Check that adding machines works correctly.
On 2012/03/13 19:23:19, niemeyer wrote:
> Sorry for nitpicking, but these comments aren't really adding any information
> that is not present in the code. Comments are great when they are correct and
> inform the reader of something that isn't otherwise trivially available.
Several
> of these one-liners are simply re-stating the method name:
> 
>     TestAddMachine == test that add machine works.
> 
> That said, the tests themselves look good here, thanks. The fact I'm
nitpicking
> rather worried about the tests themselves is great. :-)

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode149
state/state_test.go:149: machineTwo, err := s.st.AddMachine()
On 2012/03/13 17:02:43, fwereade wrote:
> I think I'd prefer to call these machine0 and machine1 for clarity's sake...

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode160
state/state_test.go:160: // Check that removing a machine doesn't fail.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode161
state/state_test.go:161: machineOne, err := s.st.AddMachine()
On 2012/03/13 17:02:43, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode179
state/state_test.go:179: // Check that reading a machine doesn't fail.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode188
state/state_test.go:188: // Check that reading a non-existent machine fails
nicely.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode199
state/state_test.go:199: // Check that reading all machines works correctly.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode218
state/state_test.go:218: // Check that adding services works correctly.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode247
state/state_test.go:247: // Check that removing the service works correctly.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode255
state/state_test.go:255: // Check that reading a non-existent service fails
nicely.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

https://codereview.appspot.com/5690051/diff/19001/state/state_test.go#newcode261
state/state_test.go:261: // Check without existing services.
On 2012/03/13 19:23:19, niemeyer wrote:
> Same.

Done.

[niemeyer, 2012-03-13 20:07:26]

LGTM, thanks.

[niemeyer, 2012-03-13 20:07:26]

LGTM, thanks.

[TheMue, 2012-03-13 20:18:32]

*** Submitted:

Continued development of the machine state.

State methods and tests for the machine state.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/5690051