Code review comment for lp:~themue/juju-core/014-deployer-lxc-context

A few initial thoughts, probably not all actually relevant at this
stage, but I want to record them. I'll do this properly when I get home.

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go
File worker/deployer/lxc.go (right):

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode20
worker/deployer/lxc.go:20: // TODO(mue) Integrate it into the deployer.
The Deployer shouldn't need any changes to use this context. Can we drop
this TODO?

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode23
worker/deployer/lxc.go:23: EnvironName string
Please make these private, to match SimpleContext.

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode28
worker/deployer/lxc.go:28: StateInfo *state.Info
I guess we can use a state.Info here, but it'd be nice to be consistent.

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode95
worker/deployer/lxc.go:95: ctx.destroyContainer(containerName)
Can this not error?

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode156
worker/deployer/lxc.go:156: return fmt.Errorf("container %q is already
created", containerName)
Surely we should destroy the pre-existing container?

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode210
worker/deployer/lxc.go:210: defer removeOnErr(&err, conf.Dir())
On 2013/03/08 21:58:04, TheMue wrote:
> On 2013/03/08 21:43:11, dimitern wrote:
> > I don't think it's clearer now - maybe it deserves a comment or you
should
> call
> > the defer earlier?

> It's a re-usage of the mechanism which SimpleContext has introduced.
Once the
> one err here in the method all defers clean up the created
directories. Using
> individual err's would let this logic fail, because only the dir where
the error
> has been caused would be removed.

I think this is risky, because every future editor needs o remember
never to return an "err" variable in its own scope.

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode239
worker/deployer/lxc.go:239: if err := lxcRun("lxc-stop", args...); err
!= nil {
Does this always work even if the container's not running?

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode252
worker/deployer/lxc.go:252: return "juju-" + ctx.EnvironName + ":" +
ctx.StateInfo.EntityName + ":" + entityName
This StateInfo.EntityName thing is ridiculous -- I know it's my fault,
but please break out a distinct deployerName as in SimpleContext.

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode260
worker/deployer/lxc.go:260: return err == nil
I don't think this is right.

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode289
worker/deployer/lxc.go:289: return false
Shouldn't we be a bit more careful about actual errors here?

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode311
worker/deployer/lxc.go:311: fcopy := func(fi os.FileInfo) error {
What if there are subdirectories?

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode319
worker/deployer/lxc.go:319: destFile, err :=
os.Create(filepath.Join(containerToolsDir, name))
Permissions?

https://codereview.appspot.com/7637045/