Code review comment for lp:~thumper/juju-core/lxc-broker

Please take a look.

https://codereview.appspot.com/10409049/diff/1/container/lxc/test.go
File container/lxc/test.go (right):

https://codereview.appspot.com/10409049/diff/1/container/lxc/test.go#newcode4
container/lxc/test.go:4: package lxc
On 2013/06/21 00:19:32, wallyworld wrote:
> This limitation of Go really annoys me.
> Could we add a big file level comment that these exported methods must
only be
> used for tests and not for normal business logic. I know each method
mentions
> the use of tests but I think a global comment would be good too.

Done.

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker.go
File worker/provisioner/lxc-broker.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker.go#newcode27
worker/provisioner/lxc-broker.go:27:
On 2013/06/21 00:19:32, wallyworld wrote:
> To help with understanding what interface lxcBroker implements, and to
cause a
> compile time error if the contract is not met, the idiom we have been
using for
> other things is to add a line like this above the struct definition:

> var _ Broker = (*lxcBroker)(nil)

> (rinse and repeat for any other implemented interfaces).

Done.

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go#newcode80
worker/provisioner/lxc-broker_test.go:80: c.Assert(lxc.Id(), Equals,
instance.Id("juju-machine-1-lxc-0"))
On 2013/06/21 00:19:32, wallyworld wrote:
> Can we test more than this? Perhaps ensure the started instance is in
the list
> of running instances? Anything else we can check? Like files/dirs
created to
> show the instance is started?

Are we getting into duplication here? Perhaps we could check that there
is a container directory with this instance id.

https://codereview.appspot.com/10409049/diff/1/worker/provisioner/lxc-broker_test.go#newcode88
worker/provisioner/lxc-broker_test.go:88: c.Assert(err, IsNil)
On 2013/06/21 00:19:32, wallyworld wrote:
> There's more we could do here - we haven't verified the stop actually
worked.
> The method could be empty and just return nil and the test would still
pass.
> Could we perhaps list the running instances and verify it's empty? Oh,
and
> invoke stop on just one instance and see that the others remain.

Done.

https://codereview.appspot.com/10409049/