Code review comment for lp:~matsubara/charms/precise/oops-tools/trunk

Revision history for this message
Diogo Matsubara (matsubara) wrote :

> Diogo, this does not look right.

Hi Clint, thanks for the review

> I see config-get's mixed with relation-get's in db-relation-changed/joined.

The way I understand the workflow is:

db-relation-joined sets the values for USER and DATABASE from the config values and pass those to the postgresql charm
Once postgresql charm finishes doing its job, it triggers the db-relation-changed and oops-tools sets its configs from those values. Before, oops-tools charm didn't have a db-relation-joined hook, so that's why there was a check if the user was set. Now that it has a joined and changed hooks, when the changed runs I can safely assume the db charm has been run.
Is this a misunderstanding on my part? Please help clarify so I can fix this.

> There is a rabbitmq charm, so I'd like to see a rabbitmq relation in addition
> to the config.yaml values for the external AMQP server.

Not sure I understand this, I added a amqp-relation-changed hook. What else is missing?

> Also the check for empty relation values seems to have been removed in db-
> relation-changed, which may cause race conditions since we may run that hook
> before the database server has set the values we want.

Explained above.



« Back to merge proposal