remove state.Tools and use tools.Tools

remove state.Tools and use tools.Tools

Mega-move, but this branch just moves environ/agent/tools.go
and tests into tools.

Also moves state bson marshalling into tools.

https://code.launchpad.net/~thumper/juju-core/move-environs-tools/+merge/173645

(do not edit description out of merge proposal)

[thumper, 2013-07-09 05:08:27]

Please take a look.

[dave_cheney.net, 2013-07-09 05:36:03]

Phew! LGTM with minor comments.

https://codereview.appspot.com/11028043/diff/1/cmd/jujud/agent_test.go
File cmd/jujud/agent_test.go (right):

https://codereview.appspot.com/11028043/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:123: c.Assert(err, IsNil)
unfortunate

https://codereview.appspot.com/11028043/diff/1/tools/marshal.go
File tools/marshal.go (right):

https://codereview.appspot.com/11028043/diff/1/tools/marshal.go#newcode16
tools/marshal.go:16: 
comment please

https://codereview.appspot.com/11028043/diff/1/tools/marshal.go#newcode23
tools/marshal.go:23: 
same

https://codereview.appspot.com/11028043/diff/1/tools/storage.go
File tools/storage.go (right):

https://codereview.appspot.com/11028043/diff/1/tools/storage.go#newcode62
tools/storage.go:62: if t.Binary, err = version.ParseBinary(vers); err != nil {
is it worthwhile adding a warning here, the only things inside the toolsPrefix
should be things that conform to the tools naming schema.

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

https://codereview.appspot.com/11028043/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:76: func (ctx *SimpleContext) DeployUnit(unitName,
initialPassword string) (err error) {
I'm a bit wary of this named return value.

https://codereview.appspot.com/11028043/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:87: defer removeOnErr(&err, toolsDir)
this removeOnErr is taking the address of the err defined in the func signature,
but it really doesn't need too.

https://codereview.appspot.com/11028043/diff/1/worker/deployer/simple.go#newc...
worker/deployer/simple.go:117: return err
this is a new err here, which normally would not work, but because deferOnErr
above is closing over the value of err in the function sig, it does work. I
think this is brittle.

[jameinel, 2013-07-09 07:37:43]

We chatted a bit on IRC, and I think we sorted out that we'd rather call it
"agent.Tools". It also matches what William would like to see, which is that
most of environs/agent becomes just a top level agent/* package that has 0
dependencies on environs. (if we have things that depend on environs we can
leave a different package there.)

So +1 on the removal of state.Tools and replacing it with something better. I'd
just like us to call that agent.Tools before we land this patch, so we don't
have to do yet-another rename-of-everything.

[dimitern, 2013-07-11 20:44:33]

LGTM, nice!