Code review comment for lp:~thumper/juju-core/machine-worker
Revision history for this message
| Tim Penhey (thumper) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
bbfa235
(Get the code!)
Please take a look.
https:/
File worker/
(right):
https:/
worker/
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:/
worker/
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:/
worker/
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:/
worker/
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:/
worker/
aptSettings := env.AptProxySet
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:/
worker/
*MachineEnviron
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:/
worker/
*MachineEnviron
On 2014/01/28 04:44:50, wallyworld wrote:
> ditto
Done.
https:/
worker/
*MachineEnviron
On 2014/01/28 04:44:50, wallyworld wrote:
> ditto
Done.
https:/
File worker/
(right):
https:/
worker/
_ = gc.Suite(
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:/