Code review comment for lp:~ev/ubuntu-ci-services-itself/restish-swift-deployment

Revision history for this message
Andy Doan (doanac) wrote :

You might want to take a look at my branch. I made an update based on your comments. Specifically i think you'll want to merge in:

 http://bazaar.launchpad.net/~doanac/ubuntu-ci-services-itself/restish-charm-local/revision/115

I also merged with trunk for revno 114. My notes:

1) The bulk of this is just adding charm-helpers. I think Joe's approach in the webui MP handles this better. He's got a makefile that pulls in a specific revno of charm-helpers. I think this will keep our source tree a lot cleaner - especially when we start to merge in all the other charms.

2) Are you sure code like:
  code_runner = config['user_code_runner']

is safe when no value is provided and default is intended? When I've done this in the past, there is no dictionary item for elements that aren't specified explicitly. so i've always had to do: config.get('user_code_runner', 'restish'). Seems odd/annoying juju wouldn't do that for me, so maybe you know a trick

3) do we really need all the swift_* config options? Now that we have started to say we want "auth config" type stuff to go into a juju-deployer/config file that we pass to the charm, I was thinking this type of data could go there, so we don't have to specify it in every charm?

« Back to merge proposal