Merge lp:~hopem/charms/precise/rabbitmq-server/lp1281753 into lp:charms/rabbitmq-server

[Edward Hope-Morley]

[Ante Karamatić (ivoks)]

configure_sources() needs to be executed during install(), not during config_changed()

review: Needs Fixing

[Marco Ceppi (marcoceppi)]

Same issues as before, make test fails because of no unit_tests, lint fails with flake8 errors, and proof fails because of missing default: for `key`

review: Needs Fixing

[Charles Butler (lazypower)]

Greetings Edward,

Thank you for the time spent working on this modification. I've reviewed the suggested modifications and have the following knitpicky comments:

when running charm proof I received a notice:
W: config.yaml: option key does not have the keys: default

I see there are other additions that use default: None - what was the purpose of leaving it out on this particular option?

The documentation of new configuration items is fully missing. I'd like to see this updated.

Thanks again, aside from the knit picks +1 LGTM

review: Approve

[Charles Butler (lazypower)]

review: Needs Fixing

[Edward Hope-Morley (hopem)]

Charles, thanks for looking at this. I've fixed up the issues you found. Not sure about the docs on config items comment though. I have added a docstring for each new config item, is that not enough? I don't see config options listed in the README.

[lp:~hopem/charms/precise/rabbitmq-server/lp1281753]

56. By Edward Hope-Morley on 2014-03-14

fixed default string values for key and source cfg options

[Matt Bruzek (mbruzek)]

Hello Edward,

Thanks for working on the rabbitmq-server charm. I ran charm proof on the charm and also get 2 warnings for missing default values in the config options you added. Normally we like to avoid warning messages from charm proof but I see the code checks for the unset condition (None) in the code.

We want the users to know how to set these new values which is usually in the README file. There is currently another merge proposal with a more descriptive README. You should consider adding some text about the new new config options in the README file so it can be merged in with the new one.

The set set source to cloud:precise-updates/havana and the charm deployed for me.

+1 LGTM.

review: Approve

[James Page (james-page)]

Hi Ed

Looking at the addition of add_source and apt_update

1) Use in config_changed

Not sure its adding any value here - it just adds the source then updates the package indexes, rather than doing any sort of upgrade. Was the intent to allow a running environment to be upgraded?

2) apt_update usage

Please call with fatal=True - if something is foo-bared we should let users know

review: Needs Fixing

[Matt Bruzek (mbruzek)]

If configuration options are added they must be handled in the config-changed hook either directly or indirectly, otherwise the changes to those configuration values will not get picked up.

review: Needs Fixing

[James Page (james-page)]

I have a revised version of this branch which switches the install steps from install to config_changed, which means that a change in source will take effect during operation of the service

For example, the rabbitmq in 12.04 can be replaced with the rabbitmq-server from the cloud-archive for icehouse.

Note that the upgrade can be racey - as it happens across all service units at the same time so if rabbit is clustered this can cause hook failures which can be resolved --retry'ed

[James Page (james-page)]

https://code.launchpad.net/~james-page/charms/precise/rabbitmq-server/source-take-two/+merge/212576

Unmerged revisions

=== modified file 'charm-helpers.yaml'
--- charm-helpers.yaml	2014-01-16 13:47:32 +0000
+++ charm-helpers.yaml	2014-03-14 12:37:00 +0000
@@ -4,3 +4,4 @@
     - core
     - contrib.charmsupport
     - contrib.openstack
+    - fetch
\ No newline at end of file

=== modified file 'config.yaml'
--- config.yaml	2014-02-18 15:51:47 +0000
+++ config.yaml	2014-03-14 12:37:00 +0000
@@ -83,3 +83,20 @@
     description: |
       If True, services that support it will log to syslog instead of their normal
       log location.
+  key:
+      type: string
+      description: |
+        Key ID to import to the apt keyring to support use with arbitary source
+        configuration from outside of Launchpad archives or PPA's.
+  source:
+      type: string
+      description: |
+        Optional configuration to support use of additional sources such as:
+        .
+        - ppa:myteam/ppa
+        - cloud:precise-proposed/folsom
+        - http://my.archive.com/ubuntu main
+        .
+        The last option should be used in conjunction with the key configuration
+        option.
+

=== modified file 'hooks/lib/utils.py'
--- hooks/lib/utils.py	2014-02-27 17:36:04 +0000
+++ hooks/lib/utils.py	2014-03-14 12:37:00 +0000
@@ -58,59 +58,6 @@
     template = templates.get_template(template_name)
     return template.render(context)
 
-CLOUD_ARCHIVE = """
-# Ubuntu Cloud Archive
-deb http://ubuntu-cloud.archive.canonical.com/ubuntu {} main
-"""
-
-CLOUD_ARCHIVE_POCKETS = {
-    'folsom': 'precise-updates/folsom',
-    'folsom/updates': 'precise-updates/folsom',
-    'folsom/proposed': 'precise-proposed/folsom',
-    'grizzly': 'precise-updates/grizzly',
-    'grizzly/updates': 'precise-updates/grizzly',
-    'grizzly/proposed': 'precise-proposed/grizzly',
-    'havana': 'precise-updates/havana',
-    'havana/updates': 'precise-updates/havana',
-    'havana/proposed': 'precise-proposed/havana'}
-
-
-def configure_source():
-    source = str(config_get('openstack-origin'))
-    if not source:
-        return
-    if source.startswith('ppa:'):
-        cmd = [
-            'add-apt-repository',
-            source]
-        subprocess.check_call(cmd)
-    if source.startswith('cloud:'):
-        # CA values should be formatted as cloud:ubuntu-openstack/pocket, eg:
-        #   cloud:precise-folsom/updates or cloud:precise-folsom/proposed
-        install('ubuntu-cloud-keyring')
-        pocket = source.split(':')[1]
-        pocket = pocket.split('-')[1]
-        with open('/etc/apt/sources.list.d/cloud-archive.list', 'w') as apt:
-            apt.write(CLOUD_ARCHIVE.format(CLOUD_ARCHIVE_POCKETS[pocket]))
-    if source.startswith('deb'):
-        l = len(source.split('|'))
-        if l == 2:
-            (apt_line, key) = source.split('|')
-            cmd = [
-                'apt-key',
-                'adv', '--keyserver keyserver.ubuntu.com',
-                '--recv-keys', key]
-            subprocess.check_call(cmd)
-        elif l == 1:
-            apt_line = source
-
-        with open('/etc/apt/sources.list.d/quantum.list', 'w') as apt:
-            apt.write(apt_line + "\n")
-    cmd = [
-        'apt-get',
-        'update']
-    subprocess.check_call(cmd)
-
 # Protocols
 TCP = 'TCP'
 UDP = 'UDP'

=== modified file 'hooks/rabbitmq_server_relations.py'
--- hooks/rabbitmq_server_relations.py	2014-02-27 17:36:04 +0000
+++ hooks/rabbitmq_server_relations.py	2014-03-14 12:37:00 +0000
@@ -20,6 +20,10 @@
 from charmhelpers.core import hookenv
 from charmhelpers.core.host import rsync
 from charmhelpers.contrib.charmsupport.nrpe import NRPE
+from charmhelpers.fetch import (
+    apt_update,
+    add_source
+)
 
 
 SERVICE_NAME = os.getenv('JUJU_UNIT_NAME').split('/')[0]
@@ -34,6 +38,10 @@
 
 
 def install():
+    # Add archive source if provided
+    add_source(utils.config_get('source'), utils.config_get('key'))
+    apt_update()
+
     pre_install_hooks()
     utils.install(*rabbit.PACKAGES)
     utils.expose(5672)
@@ -365,6 +373,10 @@
 
 
 def config_changed():
+    # Add archive source if provided
+    add_source(utils.config_get('source'), utils.config_get('key'))
+    apt_update()
+
     unison.ensure_user(user=rabbit.SSH_USER, group='rabbit')
     ensure_unison_rabbit_permissions()
 

=== modified file 'lib/charmhelpers/fetch/__init__.py'
--- lib/charmhelpers/fetch/__init__.py	2014-01-10 11:16:09 +0000
+++ lib/charmhelpers/fetch/__init__.py	2014-03-14 12:37:00 +0000
@@ -135,8 +135,12 @@
 
 
 def add_source(source, key=None):
+    if source is None:
+        log('Source is not present. Skipping')
+        return
+
     if (source.startswith('ppa:') or
-        source.startswith('http:') or
+        source.startswith('http') or
         source.startswith('deb ') or
             source.startswith('cloud-archive:')):
         subprocess.check_call(['add-apt-repository', '--yes', source])
@@ -156,7 +160,9 @@
         with open('/etc/apt/sources.list.d/proposed.list', 'w') as apt:
             apt.write(PROPOSED_POCKET.format(release))
     if key:
-        subprocess.check_call(['apt-key', 'import', key])
+        subprocess.check_call(['apt-key', 'adv', '--keyserver',
+                               'keyserver.ubuntu.com', '--recv',
+                               key])
 
 
 class SourceConfigError(Exception):