Merge lp://staging/~stub/charms/precise/pgbouncer/devel into lp://staging/charms/pgbouncer

Proposed by Stuart Bishop
Status: Merged
Merged at revision: 62
Proposed branch: lp://staging/~stub/charms/precise/pgbouncer/devel
Merge into: lp://staging/charms/pgbouncer
Prerequisite: lp://staging/~stub/charms/precise/pgbouncer/charmhelpers
Diff against target: 1645 lines (+1009/-449)
21 files modified
.bzrignore (+1/-0)
Makefile (+18/-0)
config.yaml (+11/-3)
hooks/backend-db-admin-relation-broken (+0/-20)
hooks/backend-db-admin-relation-changed (+0/-51)
hooks/config-changed (+0/-17)
hooks/db-proxy-relation-broken (+0/-9)
hooks/db-proxy-relation-changed (+0/-135)
hooks/hooks.py (+419/-0)
hooks/install (+0/-40)
hooks/nrpe-external-master-relation-changed (+0/-23)
hooks/start (+0/-20)
hooks/stop (+0/-15)
icon.svg (+339/-0)
metadata.yaml (+4/-2)
revision (+0/-1)
scripts/common (+0/-82)
templates/pgbouncer.ini.general.tmpl (+0/-31)
templates/pgbouncer.ini.tmpl (+40/-0)
tests/00-setup (+5/-0)
tests/10-basic.py (+172/-0)
To merge this branch: bzr merge lp://staging/~stub/charms/precise/pgbouncer/devel
Reviewer Review Type Date Requested Status
Cory Johns (community) Approve
charmers Pending
Review via email: mp+223741@code.staging.launchpad.net

Description of the change

Rewrite (most of) the pgbouncer charm in Python.

I've kept the same model, fixing a few minor issues like correctly setting the state and allowed-hosts relation settings.

I've left the nagios hook mostly untouched and in bash for a future branch to address.

I've added some amulet tests.

To post a comment you must log in.
Revision history for this message
Stuart Bishop (stub) wrote :

Oh, and I've flagged as deprecated a load of settings that you will never want to change and would probably break things if you did.

Revision history for this message
José Antonio Rey (jose) wrote :

Stuart,

I see that you have listed lp:~stub/charms/precise/pgbouncer/charmelpers as a prerequisite for this branch, but I do not see any open merge proposals for that branch. Is it still a prerequisite?

Revision history for this message
Stuart Bishop (stub) wrote :

The charmhelpers branch is OK to land - it is just a sync of charm-helpers code. I put it in a separate branch to make the actual pgbouncer work easier to review.

Revision history for this message
Stuart Bishop (stub) wrote :

charmhelpers branch has landed

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

Stuart,

Thank you for refactoring this charm. It's good to see it using charmhelpers, and especially good to see it have a test case. However, the test failed for me due to the fragility of the "explicitdb" check (increasing the timeout allowed it to pass). Obviously, it would be best if the juju-core bug could be fixed, but in the meantime, maybe polling the relation-get with a timeout would increase the reliability of this test?

There are also two warnings from `charm proof` related to the missing categories and icon.svg. I know they were missing before this refactor, but I think it would be worth adding the categories and just re-using the icon from the postgresql charm to get `charm proof` passing cleanly.

Other than those concerns, with the caveat of my limited domain knowledge of running pgbouncer, this refactor looks good.

80. By Stuart Bishop

Pacify charm proof

81. By Stuart Bishop

EZ charm-helpers-sync

82. By Stuart Bishop

sync charmhelpers

83. By Stuart Bishop

Improve known race condition handling in the test

Revision history for this message
Stuart Bishop (stub) wrote :

I've made 'charm proof' happy, adding the category and reusing the PostgreSQL icon (I couldn't find suitable free clipart for a different icon).

I have improved the handling of the race condition in the test as suggested.

I have pulled in a fresh copy of charm-helpers while I'm here, as I want the add_source() bug fix I just landed.

84. By Stuart Bishop

Update charm-helpers

Revision history for this message
Stuart Bishop (stub) wrote :

I've moved the fresh charm-helpers sync into the dependant branch to remove that noise from this MP.

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

Excellent. Giving this my +1 for a ~charmer to merge

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: