Merge lp:~ahasenack/landscape-charm/service-names-1379133 into lp:~landscape/landscape-charm/trunk
- service-names-1379133
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | Andreas Hasenack |
Approved revision: | 225 |
Merged at revision: | 210 |
Proposed branch: | lp:~ahasenack/landscape-charm/service-names-1379133 |
Merge into: | lp:~landscape/landscape-charm/trunk |
Diff against target: |
270 lines (+118/-22) 3 files modified
hooks/hooks.py (+46/-9) hooks/lib/util.py (+2/-2) hooks/test_hooks.py (+70/-11) |
To merge this branch: | bzr merge lp:~ahasenack/landscape-charm/service-names-1379133 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Chad Smith | Approve | ||
Benji York (community) | Approve | ||
Review via email: mp+238299@code.launchpad.net |
Commit message
Update the haproxy jinja variables in the apache vhost template files according to whatever service name haproxy was deployed as.
Description of the change
Update the haproxy jinja variables in the apache vhost template files according to whatever service name haproxy was deployed as.
The apache charm expects the jinja variables in the vhost template files to be named after the services it relates to. So if haproxy is deployed as "landscape-
An additional complication is that jinja variables need to be proper python, and the apache charm chose to swallow the "-". So we need to apply the same transformation in our side.
- 219. By Andreas Hasenack
-
Simplify regexp
- 220. By Andreas Hasenack
-
Other review comments.
Andreas Hasenack (ahasenack) : | # |
Benji York (benji) wrote : | # |
Inline comment about my memory of PEP 257 failing.
- 221. By Andreas Hasenack
-
docstrings
Chad Smith (chad.smith) wrote : | # |
[1] & [2] inline comments that are nits. I'd like to avoid truncating unacceptable characters in var names if we can replace them with underscores instead. Maybe the hyphens in service names were there for readability.
[3] Also can you run "make lint" target and sort the lint errors in this branch.
Andreas Hasenack (ahasenack) wrote : | # |
Chad, I'm doing what the apache charm already does. If haproxy is deployed as "landscape-
Chad Smith (chad.smith) : | # |
Chad Smith (chad.smith) : | # |
Chad Smith (chad.smith) wrote : | # |
minor inline docstring comment.
I'll check a full deployment with a hyphenated service name to confirm but +1 otherwise.
Chad Smith (chad.smith) wrote : | # |
validated both 400 errors on trunk and the fix. +1
- 222. By Andreas Hasenack
-
lint
- 223. By Andreas Hasenack
-
docstrings
- 224. By Andreas Hasenack
-
- haproxy_
service_ name is now a mandatory argument
- docstrings - 225. By Andreas Hasenack
-
test_no_
haproxy_ service_ name_if_ not_related_ to_haproxy( )
Preview Diff
1 | === modified file 'hooks/hooks.py' | |||
2 | --- hooks/hooks.py 2014-10-01 20:16:43 +0000 | |||
3 | +++ hooks/hooks.py 2014-10-16 12:49:51 +0000 | |||
4 | @@ -98,22 +98,51 @@ | |||
5 | 98 | services=yaml.safe_dump(_get_services_haproxy())) | 98 | services=yaml.safe_dump(_get_services_haproxy())) |
6 | 99 | 99 | ||
7 | 100 | 100 | ||
9 | 101 | def notify_vhost_config_relation(relation_id=None): | 101 | def _get_haproxy_service_name(): |
10 | 102 | """ | ||
11 | 103 | Find out what service name was used to deploy haproxy and sanitize it | ||
12 | 104 | according to the jinja requirements. The service name is used as a | ||
13 | 105 | variable name in the apache vhost jinja template. | ||
14 | 106 | |||
15 | 107 | For example, if haproxy is deployed as "landscape-haproxy", the apache | ||
16 | 108 | charm will transform that into landscapehaproxy. | ||
17 | 109 | """ | ||
18 | 110 | haproxy_relations = juju.relation_ids("website") | ||
19 | 111 | if not haproxy_relations: | ||
20 | 112 | return None | ||
21 | 113 | haproxy_relation_units = juju.relation_list(haproxy_relations[0]) | ||
22 | 114 | haproxy_service = haproxy_relation_units[0].rsplit("/", 1)[0] | ||
23 | 115 | # jinja2 templates require python-type variables, remove all characters | ||
24 | 116 | # that do not comply | ||
25 | 117 | haproxy_service = re.sub("\W", "", haproxy_service) | ||
26 | 118 | return haproxy_service | ||
27 | 119 | |||
28 | 120 | |||
29 | 121 | def _get_vhost_template(template_filename, haproxy_service_name): | ||
30 | 122 | """Expand the template with the provided haproxy service name.""" | ||
31 | 123 | with open("%s/config/%s" % (ROOT, template_filename), "r") as handle: | ||
32 | 124 | contents = handle.read() | ||
33 | 125 | contents = re.sub(r"{{ haproxy_([^}]+) }}", r"{{ %s_\1 }}" % | ||
34 | 126 | haproxy_service_name, contents) | ||
35 | 127 | return contents | ||
36 | 128 | |||
37 | 129 | |||
38 | 130 | def notify_vhost_config_relation(haproxy_service_name, relation_id=None): | ||
39 | 102 | """ | 131 | """ |
40 | 103 | Notify the vhost-config relation. | 132 | Notify the vhost-config relation. |
41 | 104 | 133 | ||
42 | 105 | This will mark it "ready to proceed". If relation_id is specified | 134 | This will mark it "ready to proceed". If relation_id is specified |
43 | 106 | use that as the relation context, otherwise look up and notify all | 135 | use that as the relation context, otherwise look up and notify all |
44 | 107 | vhost-config relations. | 136 | vhost-config relations. |
45 | 137 | |||
46 | 138 | The haproxy_service_name is needed so that the vhost template can be | ||
47 | 139 | adjusted with the correct jinja variable that apache will look for. | ||
48 | 108 | """ | 140 | """ |
49 | 109 | vhosts = [] | 141 | vhosts = [] |
57 | 110 | with open("%s/config/vhostssl.tmpl" % ROOT, 'r') as handle: | 142 | contents = _get_vhost_template("vhostssl.tmpl", haproxy_service_name) |
58 | 111 | vhosts.append({ | 143 | vhosts.append({"port": "443", "template": b64encode(contents)}) |
59 | 112 | "port": "443", "template": b64encode(handle.read())}) | 144 | contents = _get_vhost_template("vhost.tmpl", haproxy_service_name) |
60 | 113 | with open("%s/config/vhost.tmpl" % ROOT, 'r') as handle: | 145 | vhosts.append({"port": "80", "template": b64encode(contents)}) |
54 | 114 | vhosts.append({ | ||
55 | 115 | "port": "80", "template": b64encode(handle.read())}) | ||
56 | 116 | |||
61 | 117 | relation_ids = [relation_id] | 146 | relation_ids = [relation_id] |
62 | 118 | if relation_id is None: | 147 | if relation_id is None: |
63 | 119 | relation_ids = juju.relation_ids("vhost-config") | 148 | relation_ids = juju.relation_ids("vhost-config") |
64 | @@ -394,7 +423,15 @@ | |||
65 | 394 | if not juju.relation_ids("vhost-config"): | 423 | if not juju.relation_ids("vhost-config"): |
66 | 395 | return | 424 | return |
67 | 396 | 425 | ||
69 | 397 | notify_vhost_config_relation(os.environ.get("JUJU_RELATION_ID", None)) | 426 | # If we are not related to haproxy yet, noop, because we need to know the |
70 | 427 | # haproxy service name so we can set the template variable to the correct | ||
71 | 428 | # name in the apache vhost template. | ||
72 | 429 | haproxy_service_name = _get_haproxy_service_name() | ||
73 | 430 | if not haproxy_service_name: | ||
74 | 431 | return | ||
75 | 432 | |||
76 | 433 | notify_vhost_config_relation(haproxy_service_name, | ||
77 | 434 | os.environ.get("JUJU_RELATION_ID", None)) | ||
78 | 398 | 435 | ||
79 | 399 | access_details = _get_db_access_details() | 436 | access_details = _get_db_access_details() |
80 | 400 | if not access_details: | 437 | if not access_details: |
81 | 401 | 438 | ||
82 | === modified file 'hooks/lib/util.py' | |||
83 | --- hooks/lib/util.py 2014-10-01 20:16:43 +0000 | |||
84 | +++ hooks/lib/util.py 2014-10-16 12:49:51 +0000 | |||
85 | @@ -18,7 +18,7 @@ | |||
86 | 18 | Returns true if the given email is safe to use and has no "funny" | 18 | Returns true if the given email is safe to use and has no "funny" |
87 | 19 | characters. We don't go overboard and look for an RFC compliant email | 19 | characters. We don't go overboard and look for an RFC compliant email |
88 | 20 | here. | 20 | here. |
90 | 21 | 21 | ||
91 | 22 | @param email: string containing the email to be validated | 22 | @param email: string containing the email to be validated |
92 | 23 | """ | 23 | """ |
93 | 24 | valid_email_re = r"^[\w.+-]+@[\w-]+\.[\w.]+$" | 24 | valid_email_re = r"^[\w.+-]+@[\w-]+\.[\w.]+$" |
94 | @@ -103,7 +103,7 @@ | |||
95 | 103 | # let the exception be raised and stderr go through as usual. | 103 | # let the exception be raised and stderr go through as usual. |
96 | 104 | check_output(cmd, cwd="/opt/canonical/landscape", env=env) | 104 | check_output(cmd, cwd="/opt/canonical/landscape", env=env) |
97 | 105 | juju.juju_log("Administrator called %s with email %s created" % | 105 | juju.juju_log("Administrator called %s with email %s created" % |
99 | 106 | (admin_name, admin_email)) | 106 | (admin_name, admin_email)) |
100 | 107 | return True | 107 | return True |
101 | 108 | else: | 108 | else: |
102 | 109 | juju.juju_log("DB not empty, skipping first admin creation") | 109 | juju.juju_log("DB not empty, skipping first admin creation") |
103 | 110 | 110 | ||
104 | === modified file 'hooks/test_hooks.py' | |||
105 | --- hooks/test_hooks.py 2014-10-01 20:16:43 +0000 | |||
106 | +++ hooks/test_hooks.py 2014-10-16 12:49:51 +0000 | |||
107 | @@ -1,5 +1,4 @@ | |||
108 | 1 | from configobj import ConfigObj | 1 | from configobj import ConfigObj |
109 | 2 | from itertools import product | ||
110 | 3 | import base64 | 2 | import base64 |
111 | 4 | import hooks | 3 | import hooks |
112 | 5 | import mocker | 4 | import mocker |
113 | @@ -62,7 +61,10 @@ | |||
114 | 62 | Hardcode expected relation_list for tests. Feel free to expand | 61 | Hardcode expected relation_list for tests. Feel free to expand |
115 | 63 | as more tests are added. | 62 | as more tests are added. |
116 | 64 | """ | 63 | """ |
118 | 65 | return list(self._relation_list) | 64 | if relation_id == "website:1": |
119 | 65 | return ["landscape-haproxy/0"] | ||
120 | 66 | else: | ||
121 | 67 | return list(self._relation_list) | ||
122 | 66 | 68 | ||
123 | 67 | def unit_get(self, *args): | 69 | def unit_get(self, *args): |
124 | 68 | """ | 70 | """ |
125 | @@ -204,9 +206,10 @@ | |||
126 | 204 | self.mocker.replay() | 206 | self.mocker.replay() |
127 | 205 | with unittest.TestCase.assertRaises(self, ValueError) as invalid_email: | 207 | with unittest.TestCase.assertRaises(self, ValueError) as invalid_email: |
128 | 206 | hooks.util.create_landscape_admin(db_user, db_password, db_host, | 208 | hooks.util.create_landscape_admin(db_user, db_password, db_host, |
130 | 207 | admin_name, admin_email, admin_password) | 209 | admin_name, admin_email, |
131 | 210 | admin_password) | ||
132 | 208 | self.assertEqual("Invalid administrator email %s" % admin_email, | 211 | self.assertEqual("Invalid administrator email %s" % admin_email, |
134 | 209 | invalid_email.exception.message) | 212 | invalid_email.exception.message) |
135 | 210 | 213 | ||
136 | 211 | def test_first_admin_not_created_if_account_not_empty(self): | 214 | def test_first_admin_not_created_if_account_not_empty(self): |
137 | 212 | """ | 215 | """ |
138 | @@ -1575,6 +1578,43 @@ | |||
139 | 1575 | self._service_conf.seek(0) | 1578 | self._service_conf.seek(0) |
140 | 1576 | self.assertFalse(hooks._is_db_up()) | 1579 | self.assertFalse(hooks._is_db_up()) |
141 | 1577 | 1580 | ||
142 | 1581 | def test__get_haproxy_service_name(self): | ||
143 | 1582 | """ | ||
144 | 1583 | _get_haproxy_service_name() returns the jinja-ready service name used | ||
145 | 1584 | to deploy haproxy. | ||
146 | 1585 | """ | ||
147 | 1586 | haproxy_service_name = hooks._get_haproxy_service_name() | ||
148 | 1587 | self.assertEqual(haproxy_service_name, "landscapehaproxy") | ||
149 | 1588 | |||
150 | 1589 | def test_no_haproxy_service_name_if_not_related_to_haproxy(self): | ||
151 | 1590 | """ | ||
152 | 1591 | _get_haproxy_service_name() returns None if we are not related to | ||
153 | 1592 | haproxy. | ||
154 | 1593 | """ | ||
155 | 1594 | def no_website_relation(relation_name): | ||
156 | 1595 | return None | ||
157 | 1596 | |||
158 | 1597 | self.addCleanup(setattr, hooks.juju, "relation_ids", | ||
159 | 1598 | hooks.juju.relation_ids) | ||
160 | 1599 | hooks.juju.relation_ids = no_website_relation | ||
161 | 1600 | haproxy_service_name = hooks._get_haproxy_service_name() | ||
162 | 1601 | self.assertIsNone(haproxy_service_name) | ||
163 | 1602 | |||
164 | 1603 | def test__get_vhost_template(self): | ||
165 | 1604 | """ | ||
166 | 1605 | The haproxy prefix in the template variables is replaced by the | ||
167 | 1606 | name of the actual haproxy service that is part of the deployment. | ||
168 | 1607 | """ | ||
169 | 1608 | template_file = "vhostssl.tmpl" | ||
170 | 1609 | with open("%s/config/%s" % (hooks.ROOT, template_file), "r") as t: | ||
171 | 1610 | original_template = t.read() | ||
172 | 1611 | new_template = hooks._get_vhost_template(template_file, | ||
173 | 1612 | "landscape-haproxy") | ||
174 | 1613 | self.assertIn("{{ haproxy_msgserver }}", original_template) | ||
175 | 1614 | self.assertNotIn("{{ landscape-haproxy_msgserver }}", | ||
176 | 1615 | original_template) | ||
177 | 1616 | self.assertIn("{{ landscape-haproxy_msgserver }}", new_template) | ||
178 | 1617 | |||
179 | 1578 | 1618 | ||
180 | 1579 | class TestHooksServiceMock(TestHooks): | 1619 | class TestHooksServiceMock(TestHooks): |
181 | 1580 | 1620 | ||
182 | @@ -1759,7 +1799,7 @@ | |||
183 | 1759 | """ | 1799 | """ |
184 | 1760 | notify the vhost-config relation on a separate ID. | 1800 | notify the vhost-config relation on a separate ID. |
185 | 1761 | """ | 1801 | """ |
187 | 1762 | hooks.notify_vhost_config_relation("foo/0") | 1802 | hooks.notify_vhost_config_relation("haproxy", "foo/0") |
188 | 1763 | with open("%s/config/vhostssl.tmpl" % hooks.ROOT, 'r') as f: | 1803 | with open("%s/config/vhostssl.tmpl" % hooks.ROOT, 'r') as f: |
189 | 1764 | vhostssl_template = f.read() | 1804 | vhostssl_template = f.read() |
190 | 1765 | with open("%s/config/vhost.tmpl" % hooks.ROOT, 'r') as f: | 1805 | with open("%s/config/vhost.tmpl" % hooks.ROOT, 'r') as f: |
191 | @@ -1772,7 +1812,7 @@ | |||
192 | 1772 | 1812 | ||
193 | 1773 | def test_notify_vhost_config_relation(self): | 1813 | def test_notify_vhost_config_relation(self): |
194 | 1774 | """notify the vhost-config relation on the "current" ID.""" | 1814 | """notify the vhost-config relation on the "current" ID.""" |
196 | 1775 | hooks.notify_vhost_config_relation() | 1815 | hooks.notify_vhost_config_relation("haproxy") |
197 | 1776 | with open("%s/config/vhostssl.tmpl" % hooks.ROOT, 'r') as f: | 1816 | with open("%s/config/vhostssl.tmpl" % hooks.ROOT, 'r') as f: |
198 | 1777 | vhostssl_template = f.read() | 1817 | vhostssl_template = f.read() |
199 | 1778 | with open("%s/config/vhost.tmpl" % hooks.ROOT, 'r') as f: | 1818 | with open("%s/config/vhost.tmpl" % hooks.ROOT, 'r') as f: |
200 | @@ -1802,7 +1842,7 @@ | |||
201 | 1802 | "user": "user", | 1842 | "user": "user", |
202 | 1803 | "password": "password"}}) | 1843 | "password": "password"}}) |
203 | 1804 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) | 1844 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) |
205 | 1805 | notify_vhost(None) | 1845 | notify_vhost(hooks._get_haproxy_service_name(), None) |
206 | 1806 | self.mocker.replay() | 1846 | self.mocker.replay() |
207 | 1807 | self.assertRaises(SystemExit, hooks.vhost_config_relation_changed) | 1847 | self.assertRaises(SystemExit, hooks.vhost_config_relation_changed) |
208 | 1808 | self.assertIn('Waiting for data from apache', hooks.juju._logs[-1]) | 1848 | self.assertIn('Waiting for data from apache', hooks.juju._logs[-1]) |
209 | @@ -1820,7 +1860,7 @@ | |||
210 | 1820 | "user": "user", | 1860 | "user": "user", |
211 | 1821 | "password": "password"}}) | 1861 | "password": "password"}}) |
212 | 1822 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) | 1862 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) |
214 | 1823 | notify_vhost(None) | 1863 | notify_vhost(hooks._get_haproxy_service_name(), None) |
215 | 1824 | is_db_up = self.mocker.replace(hooks._is_db_up) | 1864 | is_db_up = self.mocker.replace(hooks._is_db_up) |
216 | 1825 | is_db_up() | 1865 | is_db_up() |
217 | 1826 | self.mocker.result(False) | 1866 | self.mocker.result(False) |
218 | @@ -1843,7 +1883,7 @@ | |||
219 | 1843 | "user": "user", | 1883 | "user": "user", |
220 | 1844 | "password": "password"}}) | 1884 | "password": "password"}}) |
221 | 1845 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) | 1885 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) |
223 | 1846 | notify_vhost(None) | 1886 | notify_vhost(hooks._get_haproxy_service_name(), None) |
224 | 1847 | is_db_up = self.mocker.replace(hooks._is_db_up) | 1887 | is_db_up = self.mocker.replace(hooks._is_db_up) |
225 | 1848 | is_db_up() | 1888 | is_db_up() |
226 | 1849 | self.mocker.result(True) | 1889 | self.mocker.result(True) |
227 | @@ -1868,7 +1908,7 @@ | |||
228 | 1868 | "user": "user", | 1908 | "user": "user", |
229 | 1869 | "password": "password"}}) | 1909 | "password": "password"}}) |
230 | 1870 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) | 1910 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) |
232 | 1871 | notify_vhost(None) | 1911 | notify_vhost(hooks._get_haproxy_service_name(), None) |
233 | 1872 | mock_conn = self.mocker.mock() | 1912 | mock_conn = self.mocker.mock() |
234 | 1873 | mock_conn.close() | 1913 | mock_conn.close() |
235 | 1874 | connect_exclusive = self.mocker.replace(hooks.util.connect_exclusive) | 1914 | connect_exclusive = self.mocker.replace(hooks.util.connect_exclusive) |
236 | @@ -1906,7 +1946,7 @@ | |||
237 | 1906 | "user": "user", | 1946 | "user": "user", |
238 | 1907 | "password": "password"}}) | 1947 | "password": "password"}}) |
239 | 1908 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) | 1948 | notify_vhost = self.mocker.replace(hooks.notify_vhost_config_relation) |
241 | 1909 | notify_vhost(None) | 1949 | notify_vhost(hooks._get_haproxy_service_name(), None) |
242 | 1910 | is_db_up = self.mocker.replace(hooks._is_db_up) | 1950 | is_db_up = self.mocker.replace(hooks._is_db_up) |
243 | 1911 | is_db_up() | 1951 | is_db_up() |
244 | 1912 | self.mocker.result(True) | 1952 | self.mocker.result(True) |
245 | @@ -1926,6 +1966,25 @@ | |||
246 | 1926 | with open(hooks.SSL_CERT_LOCATION, 'r') as f: | 1966 | with open(hooks.SSL_CERT_LOCATION, 'r') as f: |
247 | 1927 | self.assertEqual("foobar", f.read()) | 1967 | self.assertEqual("foobar", f.read()) |
248 | 1928 | 1968 | ||
249 | 1969 | def test_vhost_config_relation_exits_if_haproxy_not_ready(self): | ||
250 | 1970 | """ | ||
251 | 1971 | notify_vhost_config_relation() is not called if the haproxy relation | ||
252 | 1972 | is not there. | ||
253 | 1973 | """ | ||
254 | 1974 | def should_not_be_here(*args): | ||
255 | 1975 | raise AssertionError("notify_vhost_config_relation() should not " | ||
256 | 1976 | "be called") | ||
257 | 1977 | |||
258 | 1978 | self.addCleanup(setattr, hooks, "vhost_config_relation_changed", | ||
259 | 1979 | hooks.vhost_config_relation_changed) | ||
260 | 1980 | hooks.notify_vhost_config_relation = should_not_be_here | ||
261 | 1981 | get_haproxy_service_name = self.mocker.replace( | ||
262 | 1982 | hooks._get_haproxy_service_name) | ||
263 | 1983 | get_haproxy_service_name() | ||
264 | 1984 | self.mocker.result(None) | ||
265 | 1985 | self.mocker.replay() | ||
266 | 1986 | hooks.vhost_config_relation_changed() | ||
267 | 1987 | |||
268 | 1929 | 1988 | ||
269 | 1930 | class TestHooksUtils(TestHooks): | 1989 | class TestHooksUtils(TestHooks): |
270 | 1931 | def test__setup_apache(self): | 1990 | def test__setup_apache(self): |
This branch looks good. I had one regex simplification and a couple of style nitpicks, but once you consider those you're good to go.