Code review comment for lp:~johnsca/charms/trusty/cf-cloud-controller/refactor

Revision history for this message
Cory Johns (johnsca) wrote :

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.yml#newcode57
> 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/cloudfoundry/contexts.py#newcode29.
  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.

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

« Back to merge proposal