Please take a look.


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"
On 2014/01/28 04:44:50, wallyworld wrote:
> 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.

The whole point it is a variable is so we can mock it out during
testing.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode50
worker/machineenvironmentworker/machineenvironmentworker.go:50: first
         bool
On 2014/01/28 04:44:50, wallyworld wrote:
> 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.

I'll add a comment.  It is only used for testing in order to call the
Started function (which is by default a no-op).  The Started function is
mocked out in the tests so I can demonstrate that not only do the
changes happen when it starts up, but that they happen once it is in the
main loop.

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.
On 2014/01/28 04:44:50, wallyworld wrote:
> 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"??

Yes, you are right. Changed.

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
On 2014/01/28 04:44:50, wallyworld wrote:
> s/writting/written  ?

Done.

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

Yeah I was tossing up on this one as I was writing it.

I'll split it up.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode137
worker/machineenvironmentworker/machineenvironmentworker.go:137: func (w
*MachineEnvironmentWorker) SetUp() (watcher.NotifyWatcher, error) {
On 2014/01/28 04:44:50, wallyworld wrote:
> Typically we add a doc string saying what interface SetUp() comes from

cleaner, logger and machiner all didn't. I looked at your one for the
boilerplate.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode149
worker/machineenvironmentworker/machineenvironmentworker.go:149: func (w
*MachineEnvironmentWorker) Handle() error {
On 2014/01/28 04:44:50, wallyworld wrote:
> ditto

Done.

https://codereview.appspot.com/57600043/diff/1/worker/machineenvironmentworker/machineenvironmentworker.go#newcode153
worker/machineenvironmentworker/machineenvironmentworker.go:153: func (w
*MachineEnvironmentWorker) TearDown() error {
On 2014/01/28 04:44:50, wallyworld wrote:
> ditto

Done.

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{})
On 2014/01/28 04:44:50, wallyworld wrote:
> 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
> :-)

Almost all the other workers don't test this.  I was just being more
thorough.

The purpose of the first attribute is to allow isolated testing of the
SetUp, and the Loop.  The first attribute is needed to potentially write
out empty values, as now documented in the main file.

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