Code review comment for lp://staging/~themue/golxc/001-added-sudo-and-first-test

Revision history for this message
William Reade (fwereade) wrote :

A few more comments...

https://codereview.appspot.com/6852065/diff/6002/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode180
golxc.go:180: // Unfreeze thaws all a froozen container's processes.
s/rooz/roz/

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode186
golxc.go:186: return fmt.Errorf("container %q is not froozen", c.name)
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode219
golxc.go:219: switch len(states) {
I'm not sure the switch is a big win over a simple err return when no
states are supplied -- the one-state case may be more efficient but I'm
not sure it really aids clarity. Up to you though.

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode271
golxc.go:271: // String returns a brief information about the container.
s/a //

?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode25
golxc_test.go:25: c.Assert(lc.IsConstructed(), Equals, true)
Shouldn't we defer the Destroy at this point?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode40
golxc_test.go:40: defer lc1.Destroy()
This should be either 1 or 2 lines down, shouldn't it? And surely we
should be asserting no error?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode74
golxc_test.go:74: defer lc.Destroy()
Handle errors? (after checking no error on preceding Create)

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode87
golxc_test.go:87: defer lc1.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode91
golxc_test.go:91: defer lc2.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode116
golxc_test.go:116: defer lc.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode137
golxc_test.go:137: defer lc.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode138
golxc_test.go:138: go lc.Start("", "")
Destroy works even if the container is running?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode161
golxc_test.go:161: // Test that a not running container can't be
froozen.
s/rooz/roz/

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode171
golxc_test.go:171: // Test that a non-existing container can't be
froozen.
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode179
golxc_test.go:179: // Test that a non-existing container can't be
unfroozen.
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode186
golxc_test.go:186: func (l *LXCSuite) TestUnfreezeNotFroozen(c *C) {
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode187
golxc_test.go:187: // Test that a running container can't be unfroozen.
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode193
golxc_test.go:193: defer lc.Stop()
error checking

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode195
golxc_test.go:195: c.Assert(err, ErrorMatches, "container .* is not
froozen")
ditto

https://codereview.appspot.com/6852065/

« Back to merge proposal