cf-charmers team mailing list archive

[Benjamin Saller <benjamin.saller@xxxxxxxxxxxxx>]

LGTM, thanks


https://codereview.appspot.com/91450050/diff/20001/hooks/install
File hooks/install (right):

https://codereview.appspot.com/91450050/diff/20001/hooks/install#newcode60
hooks/install:60: common.FOG_CONNECTION,
On 2014/05/19 10:19:34, lomov.as wrote:
> I can't get why you called it common and not config, and why we can't
have
> separate file for all this config options to parse it using
ConfigParser or
> similar tool.

I'd say that those settings are not admin addressable (or they would be
service configration)  so we don't need a parser as a layer of
indirection around them.

As for config/common naming, I don't mind either as long as we keep it
consistent.

https://codereview.appspot.com/91450050/diff/40001/hooks/common.py
File hooks/common.py (right):

https://codereview.appspot.com/91450050/diff/40001/hooks/common.py#newcode34
hooks/common.py:34: #TODO: make it idempotent by deleting existing db if
exists
Not sure its a migration if we delete the db :)

https://codereview.appspot.com/91450050/diff/40001/hooks/hooks.py
File hooks/hooks.py (left):

https://codereview.appspot.com/91450050/diff/40001/hooks/hooks.py#oldcode44
hooks/hooks.py:44:
This will make more sense w/o sqlite as you said on IRC, today its lossy

https://codereview.appspot.com/91450050/diff/40001/metadata.yaml
File metadata.yaml (right):

https://codereview.appspot.com/91450050/diff/40001/metadata.yaml#newcode22
metadata.yaml:22: optional: true
Thanks for adding this

https://codereview.appspot.com/91450050/

-- 
https://code.launchpad.net/~johnsca/charms/trusty/cf-cloud-controller/refactor/+merge/219919
Your team Cloud Foundry Charmers is requested to review the proposed merge of lp:~johnsca/charms/trusty/cf-cloud-controller/refactor into lp:~cf-charmers/charms/trusty/cf-cloud-controller/trunk.