Code review comment for lp:~thumper/juju-core/machine-worker

Looks pretty good but we need a juju/machine_test to test that the jujud
has started the worker properly. There's a few examples in machine_test
to look at.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go
File worker/machineenvironmentworker/machineenvironmentworker.go
(right):

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode30
worker/machineenvironmentworker/machineenvironmentworker.go:30:
ProxyDirectory = "/home/ubuntu"
Not sure if other workers use ~ubuntu and/or /home/ubuntu, but if they
do , would be good to make this a const or method call.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode50
worker/machineenvironmentworker/machineenvironmentworker.go:50: first
         bool
Do other workers use first? Don't they just suck up the initial event
and deal with it? It seems a little complicated to use it here. If we do
need to use it, perhaps a comment saying why we need it here.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode77
worker/machineenvironmentworker/machineenvironmentworker.go:77: // an
ubuntu user, it will. This shouldn't be a problem.
I read the above a few times and am still not sure I've parsed it
correctly. I think "should" might need to be "shouldn't"??

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode85
worker/machineenvironmentworker/machineenvironmentworker.go:85: // same
way that the file is writting in the cloud-init process, so
s/writting/written ?

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode122
worker/machineenvironmentworker/machineenvironmentworker.go:122:
aptSettings := env.AptProxySettings()
not sure if we want the 2 lots of logic in this method in separate sub
methods so onChange is dead easy to ready

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode137
worker/machineenvironmentworker/machineenvironmentworker.go:137: func (w
*MachineEnvironmentWorker) SetUp() (watcher.NotifyWatcher, error) {
Typically we add a doc string saying what interface SetUp() comes from

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode149
worker/machineenvironmentworker/machineenvironmentworker.go:149: func (w
*MachineEnvironmentWorker) Handle() error {
ditto

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode153
worker/machineenvironmentworker/machineenvironmentworker.go:153: func (w
*MachineEnvironmentWorker) TearDown() error {
ditto

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker_test.go
File worker/machineenvironmentworker/machineenvironmentworker_test.go
(right):

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker_test.go#newcode41
worker/machineenvironmentworker/machineenvironmentworker_test.go:41: var
_ = gc.Suite(&MachineEnvironmentWatcherSuite{})
I can't see a test that explicitly check that receiving the initial
event does what is needed ie the business logic associated with the
"first" attribute. If it is there, perhaps a comment to show where it is
tested. Is that what TestInitialState does? I'd actually prefer we
didn't need the "first" attribute :-)

https://codereview.appspot.com/57600043/