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

Much better, thanks. There are a few quirks which I still cannot quite
get though.

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

https://codereview.appspot.com/7637045/diff/1/worker/deployer/lxc.go#newcode278
worker/deployer/lxc.go:278: func (ctx *LxcContext)
createConfigLink(unitName string) error {
On 2013/03/08 21:25:28, TheMue wrote:
> On 2013/03/08 20:25:32, dimitern wrote:
> > combine common parts of this, previous and next method in a single
helper
> > please.
> > Maybe even make container its own type with methods?

> Will think about it. As it not reusable I do not see much value right
now. But
> any other opinions showing me advantages are welcome.

Well, you have these 2 exact lines repeated 3 times:
containerName := ctx.containerName(unitName)
autoConfigPath := filepath.Join(ctx.AutoDir, containerName+".conf")

https://codereview.appspot.com/7637045/diff/1/worker/deployer/lxc.go#newcode328
worker/deployer/lxc.go:328: fcopy := func(fi os.FileInfo) error {
On 2013/03/08 21:25:28, TheMue wrote:
> On 2013/03/08 20:25:32, dimitern wrote:
> > it might be better to extract this into a separate free func.

> Why would this help?

Just a thought, it might make the code a bit easier to read. But I don't
insist on it.

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

https://codereview.appspot.com/7637045/diff/1/worker/deployer/lxc_test.go#newcode10
worker/deployer/lxc_test.go:10:
On 2013/03/08 21:25:28, TheMue wrote:
> On 2013/03/08 20:25:32, dimitern wrote:
> > d

> IMHO it helps to get a quicker view of what project and what standard
or 3rd
> party packages we use. But yes, gocheck is 3rd party, so I'll move it.

Fair enough, it's a matter of taste.

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#newcode190
worker/deployer/lxc.go:190: if err = copyTools(hostToolsDir, toolsDir);
err != nil {
I see no point in reusing err here - err := should be just fine.

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode193
worker/deployer/lxc.go:193: if _, err =
agent.ChangeAgentTools(ctx.HostDataDir, entityName, version.Current);
err != nil {
ditto

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode207
worker/deployer/lxc.go:207: if err = conf.Write(); err != nil {
ditto

https://codereview.appspot.com/7637045/diff/7001/worker/deployer/lxc.go#newcode210
worker/deployer/lxc.go:210: defer removeOnErr(&err, conf.Dir())
I don't think it's clearer now - maybe it deserves a comment or you
should call the defer earlier?

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