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

Proposed by Edward Hope-Morley on 2014-03-04
Status: Rejected
Rejected by: James Page on 2014-03-25
Proposed branch: lp:~hopem/charms/precise/rabbitmq-server/lp1281753
Merge into: lp:charms/rabbitmq-server
Diff against target: 164 lines (+38/-55)
5 files modified
charm-helpers.yaml (+1/-0)
config.yaml (+17/-0)
hooks/lib/utils.py (+0/-53)
hooks/rabbitmq_server_relations.py (+12/-0)
lib/charmhelpers/fetch/__init__.py (+8/-2)
To merge this branch: bzr merge lp:~hopem/charms/precise/rabbitmq-server/lp1281753
Reviewer Review Type Date Requested Status
Matt Bruzek (community) Needs Fixing on 2014-03-19
James Page 2014-03-04 Needs Fixing on 2014-03-19
Ante Karamatić 2014-03-04 Pending
Marco Ceppi 2014-03-04 Pending
Charles Butler 2014-03-04 Pending
Review via email: mp+209320@code.launchpad.net

This proposal supersedes a proposal from 2014-02-20.

To post a comment you must log in.
Ante Karamatić (ivoks) wrote : Posted in a previous version of this proposal

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

review: Needs Fixing
Marco Ceppi (marcoceppi) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) : Posted in a previous version of this proposal
review: Needs Fixing
Edward Hope-Morley (hopem) wrote : Posted in a previous version of this proposal

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.

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

fixed default string values for key and source cfg options

Matt Bruzek (mbruzek) wrote :

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) wrote :

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) wrote :

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) wrote :

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

Unmerged revisions

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charm-helpers.yaml'
2--- charm-helpers.yaml 2014-01-16 13:47:32 +0000
3+++ charm-helpers.yaml 2014-03-14 12:37:00 +0000
4@@ -4,3 +4,4 @@
5 - core
6 - contrib.charmsupport
7 - contrib.openstack
8+ - fetch
9\ No newline at end of file
10
11=== modified file 'config.yaml'
12--- config.yaml 2014-02-18 15:51:47 +0000
13+++ config.yaml 2014-03-14 12:37:00 +0000
14@@ -83,3 +83,20 @@
15 description: |
16 If True, services that support it will log to syslog instead of their normal
17 log location.
18+ key:
19+ type: string
20+ description: |
21+ Key ID to import to the apt keyring to support use with arbitary source
22+ configuration from outside of Launchpad archives or PPA's.
23+ source:
24+ type: string
25+ description: |
26+ Optional configuration to support use of additional sources such as:
27+ .
28+ - ppa:myteam/ppa
29+ - cloud:precise-proposed/folsom
30+ - http://my.archive.com/ubuntu main
31+ .
32+ The last option should be used in conjunction with the key configuration
33+ option.
34+
35
36=== modified file 'hooks/lib/utils.py'
37--- hooks/lib/utils.py 2014-02-27 17:36:04 +0000
38+++ hooks/lib/utils.py 2014-03-14 12:37:00 +0000
39@@ -58,59 +58,6 @@
40 template = templates.get_template(template_name)
41 return template.render(context)
42
43-CLOUD_ARCHIVE = """
44-# Ubuntu Cloud Archive
45-deb http://ubuntu-cloud.archive.canonical.com/ubuntu {} main
46-"""
47-
48-CLOUD_ARCHIVE_POCKETS = {
49- 'folsom': 'precise-updates/folsom',
50- 'folsom/updates': 'precise-updates/folsom',
51- 'folsom/proposed': 'precise-proposed/folsom',
52- 'grizzly': 'precise-updates/grizzly',
53- 'grizzly/updates': 'precise-updates/grizzly',
54- 'grizzly/proposed': 'precise-proposed/grizzly',
55- 'havana': 'precise-updates/havana',
56- 'havana/updates': 'precise-updates/havana',
57- 'havana/proposed': 'precise-proposed/havana'}
58-
59-
60-def configure_source():
61- source = str(config_get('openstack-origin'))
62- if not source:
63- return
64- if source.startswith('ppa:'):
65- cmd = [
66- 'add-apt-repository',
67- source]
68- subprocess.check_call(cmd)
69- if source.startswith('cloud:'):
70- # CA values should be formatted as cloud:ubuntu-openstack/pocket, eg:
71- # cloud:precise-folsom/updates or cloud:precise-folsom/proposed
72- install('ubuntu-cloud-keyring')
73- pocket = source.split(':')[1]
74- pocket = pocket.split('-')[1]
75- with open('/etc/apt/sources.list.d/cloud-archive.list', 'w') as apt:
76- apt.write(CLOUD_ARCHIVE.format(CLOUD_ARCHIVE_POCKETS[pocket]))
77- if source.startswith('deb'):
78- l = len(source.split('|'))
79- if l == 2:
80- (apt_line, key) = source.split('|')
81- cmd = [
82- 'apt-key',
83- 'adv', '--keyserver keyserver.ubuntu.com',
84- '--recv-keys', key]
85- subprocess.check_call(cmd)
86- elif l == 1:
87- apt_line = source
88-
89- with open('/etc/apt/sources.list.d/quantum.list', 'w') as apt:
90- apt.write(apt_line + "\n")
91- cmd = [
92- 'apt-get',
93- 'update']
94- subprocess.check_call(cmd)
95-
96 # Protocols
97 TCP = 'TCP'
98 UDP = 'UDP'
99
100=== modified file 'hooks/rabbitmq_server_relations.py'
101--- hooks/rabbitmq_server_relations.py 2014-02-27 17:36:04 +0000
102+++ hooks/rabbitmq_server_relations.py 2014-03-14 12:37:00 +0000
103@@ -20,6 +20,10 @@
104 from charmhelpers.core import hookenv
105 from charmhelpers.core.host import rsync
106 from charmhelpers.contrib.charmsupport.nrpe import NRPE
107+from charmhelpers.fetch import (
108+ apt_update,
109+ add_source
110+)
111
112
113 SERVICE_NAME = os.getenv('JUJU_UNIT_NAME').split('/')[0]
114@@ -34,6 +38,10 @@
115
116
117 def install():
118+ # Add archive source if provided
119+ add_source(utils.config_get('source'), utils.config_get('key'))
120+ apt_update()
121+
122 pre_install_hooks()
123 utils.install(*rabbit.PACKAGES)
124 utils.expose(5672)
125@@ -365,6 +373,10 @@
126
127
128 def config_changed():
129+ # Add archive source if provided
130+ add_source(utils.config_get('source'), utils.config_get('key'))
131+ apt_update()
132+
133 unison.ensure_user(user=rabbit.SSH_USER, group='rabbit')
134 ensure_unison_rabbit_permissions()
135
136
137=== modified file 'lib/charmhelpers/fetch/__init__.py'
138--- lib/charmhelpers/fetch/__init__.py 2014-01-10 11:16:09 +0000
139+++ lib/charmhelpers/fetch/__init__.py 2014-03-14 12:37:00 +0000
140@@ -135,8 +135,12 @@
141
142
143 def add_source(source, key=None):
144+ if source is None:
145+ log('Source is not present. Skipping')
146+ return
147+
148 if (source.startswith('ppa:') or
149- source.startswith('http:') or
150+ source.startswith('http') or
151 source.startswith('deb ') or
152 source.startswith('cloud-archive:')):
153 subprocess.check_call(['add-apt-repository', '--yes', source])
154@@ -156,7 +160,9 @@
155 with open('/etc/apt/sources.list.d/proposed.list', 'w') as apt:
156 apt.write(PROPOSED_POCKET.format(release))
157 if key:
158- subprocess.check_call(['apt-key', 'import', key])
159+ subprocess.check_call(['apt-key', 'adv', '--keyserver',
160+ 'keyserver.ubuntu.com', '--recv',
161+ key])
162
163
164 class SourceConfigError(Exception):

Subscribers

People subscribed via source and target branches