Code review comment for lp:~johnsca/charm-helpers/charm-helpers

Revision history for this message
Benjamin Saller (bcsaller) wrote :

LGTM w/minors

While not a huge issue, I really would like something better done with
the chownr method. Still ok with a shell out there.
Thanks for this, feel free to land even if all you decide to do here is
change Exception to OSError, but if you'd like me to look at the change
again, happy to.

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/contrib/cloudfoundry/contexts.py
File charmhelpers/contrib/cloudfoundry/contexts.py (right):

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/contrib/cloudfoundry/contexts.py#newcode38
charmhelpers/contrib/cloudfoundry/contexts.py:38: ctx['db']['port'] =
'3306'
s/'db'/self.interface/ on both lines

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/core/host.py
File charmhelpers/core/host.py (right):

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/core/host.py#newcode333
charmhelpers/core/host.py:333: if fatal:
This means that something could go wrong and we continue blindly. I
don't think this is what we want. If nothing else we'd only want to
catch errors related to things like symbolic links here and not things
like the path doesn't exist (maybe an upstart job is modifying the
filesytem as well for example) which Exception will catch.

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/core/services.py
File charmhelpers/core/services.py (right):

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/core/services.py#newcode53
charmhelpers/core/services.py:53: manager_type = service.get('type',
UpstartService)
Thanks for adding this, I think this is a good solution.

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/core/templating.py
File charmhelpers/core/templating.py (right):

https://codereview.appspot.com/92420046/diff/60001/charmhelpers/core/templating.py#newcode7
charmhelpers/core/templating.py:7: LOADER = None
No longer used? I think we can remove it

https://codereview.appspot.com/92420046/

« Back to merge proposal