New Charm: pgbouncer

Hello Haw! Thanks for submitting this charm, it will be a great addition to the charm store.

***
Blockers:

[1]* Since pgbouncer speaks pgsql, it shouldn't require any special interface like 'pgbouncer' to relate to it. Because of that, you can just use 'interface' pgsql on both sides.

[2]* Both relations being called 'db' may seem to "work", but its actually a bug if it even works. The hooks you have, db-relation-* are doing things for the different sides of the relation, and so are doing things in a very confusing way. While relations are always directional, so you can have this situation without an error, it doesn't actually speak to what you are providing or requiring. Since it is a proxy, I'd change the relations to:

provides:
  db-proxy:
    interface: pgsql
requires:
  backend-db:
    interface: pgsql

db-proxy should implement relation-sets that mimic the behavior of the pgsql charm, using relation-set to tell clients how to connect. Backend-db should work more or less like your current 'db' relations.
***
Minors:

scripts/common:update_config

[3]
    # Copy existing config file to preserve permissions.
    cp -a $pgbouncer_config ${pgbouncer_config}.new

This is a predictable temp file creation. While /etc/pgbouncer should remain root-writable-only, if that changed, this would allow any user who can write a symlink in that dir to overwrite files as root. Would suggest using 'mktemp' to generate a temp file and then rename it over top of the existing file.

[4]
Also, cheetah has a command line tool that can do what the template-generate.py does, perhaps consider using that to avoid more code. :)

scripts/common:update_init_script

[5]
This seems unnecessary. You can very easily set the options in /etc/default/pgbouncer, including the ulimit. The advantage there is that you can just write the whole file instead of having to do any sed gymnastics.

[6]
hooks/start,stop:

These are not idempotent. You need to not fail if the service is already started in start, and likewise, don't fail if it is already stopped in stop.

[7]
hooks/upgrade-charm:

one could argue that the charm would be simpler if you just symlinked upgrade-charm -> install and added a 'service pgbouncer reload' to the bottom of install.

BTW Haw, the next step when those issues are addressed in your branch is to change the status to 'Fix Committed' so it re-appears on our sponsorship/review queue. Thanks again for submitting the charm!

Hi Clint,

Thank you for your valuable feedback, I'll work on addressing the issues outlined above.

Regards,

Haw

Hi charmers,

comments 1-3 and 5-7 from
https://bugs.launchpad.net/charms/+bug/1046318/comments/1

were addressed.

Please pull from

https://code.launchpad.net/~alexlist/charms/precise/pgbouncer/trunk

for a fresh review. Thanks!

Note:

charm proof gives a warning, I have added myself as a second maintainer and left Haw in there as well.

Reviewing this ( https://code.launchpad.net/~alexlist/charms/precise/pgbouncer/trunk ) now.

-Juan

Approved and promulgated.

Thanks Alexander.

-Juan