Refactored to use refactored charm-helpers

Refactored to use refactored charm-helpers



https://code.launchpad.net/~johnsca/charms/trusty/cf-cloud-controller/refactor/+merge/219919

(do not edit description out of merge proposal)

[johnsca, 2014-05-16 23:10:22]

Please take a look.

[johnsca, 2014-05-16 23:33:55]

Please take a look.

[lomov.as, 2014-05-19 10:19:34]

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,
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.

https://codereview.appspot.com/91450050/diff/20001/hooks/install#newcode69
hooks/install:69: common.db_migrate()
I saw you added mysql relation to metadata.yml. It will be really cool to have
opportunity to connect it. Yay! But how and when are you going create/migrate
database for it? Also the question with config template is still open.

https://codereview.appspot.com/91450050/diff/20001/templates/cloud_controller...
File templates/cloud_controller.yml (right):

https://codereview.appspot.com/91450050/diff/20001/templates/cloud_controller...
templates/cloud_controller.yml:57: database: {{db['dsn']}}
Is it the last version? I'm not 100% sure it will work with changes in
relations. how do you plan to connect mysql relation here?

[cory.johns, 2014-05-19 13:15:13]

On 2014/05/19 10:19:34, lomov.as wrote:
> https://codereview.appspot.com/91450050/diff/20001/hooks/install#newcode60
> hooks/install:60: common.FOG_CONNECTION,
> 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.

It was changed to common because it now has a shared method in it, db_migrate. 
I agree that the large number of static fields, most of which aren't used or
aren't used more than once, should be refactored away.  I think most of it could
probably live in the templates.


> https://codereview.appspot.com/91450050/diff/20001/hooks/install#newcode69
> hooks/install:69: common.db_migrate()
> I saw you added mysql relation to metadata.yml. It will be really cool to have
> opportunity to connect it. Yay! But how and when are you going create/migrate
> database for it? Also the question with config template is still open.

The create / migrate is handled the same way as it was for sqlite, in
db_migrate.  Indeed, we even discussed creating a fallback StaticContext that
generates the old sqlite config so that it would continue to use sqlite until a
mysql charm was connected, but it wasn't clear if there was sufficient value in
that, and it raised the issue of potentially having data in sqlite that wouldn't
be migrated over to the newly connected mysql.


>
https://codereview.appspot.com/91450050/diff/20001/templates/cloud_controller...
> templates/cloud_controller.yml:57: database: {{db['dsn']}}
> Is it the last version? I'm not 100% sure it will work with changes in
> relations. how do you plan to connect mysql relation here?

See the MysqlDSNContext in
https://codereview.appspot.com/91450050/diff/20001/hooks/charmhelpers/contrib....
 It builds the MySQL DSN from the relation data and the context is used
(https://codereview.appspot.com/91450050/diff/20001/hooks/hooks.py#newcode26) to
populate the DSN in the template.

[johnsca, 2014-05-19 17:32:00]

Please take a look.

[benjamin.saller, 2014-05-19 18:52:05]

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

[cory.johns, 2014-05-19 19:20:32]

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 18:52:05, benjamin.saller wrote:
> As for config/common naming, I don't mind either as long as we keep it
> consistent.

The reason we renamed it to common was that it had the shared db_migrate method
in it.  With the removal of the partial sqlite support, that could be moved into
hooks.py and the impetus for naming it common is moot.  So perhaps I will change
it back to be consistent with the other charms.

[benjamin.saller, 2014-05-19 19:42:55]

On 2014/05/19 19:20:32, cory.johns wrote:
> 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 18:52:05, benjamin.saller wrote:
> > As for config/common naming, I don't mind either as long as we keep it
> > consistent.
> 
> The reason we renamed it to common was that it had the shared db_migrate
method
> in it.  With the removal of the partial sqlite support, that could be moved
into
> hooks.py and the impetus for naming it common is moot.  So perhaps I will
change
> it back to be consistent with the other charms.

Sounds good, thanks

[johnsca, 2014-05-20 22:12:01]

*** Submitted:

Refactored to use refactored charm-helpers

R=lomov.as, cory.johns, benjamin.saller
CC=
https://codereview.appspot.com/91450050