Merge lp:~ajkavanagh/charm-helpers/fix-unit-paused-set-functions into lp:charm-helpers
- fix-unit-paused-set-functions
- Merge into devel
Proposed by
Alex Kavanagh
Status: | Merged |
---|---|
Merged at revision: | 541 |
Proposed branch: | lp:~ajkavanagh/charm-helpers/fix-unit-paused-set-functions |
Merge into: | lp:charm-helpers |
Diff against target: |
209 lines (+68/-22) 2 files modified
charmhelpers/contrib/openstack/utils.py (+10/-6) tests/contrib/openstack/test_openstack_utils.py (+58/-16) |
To merge this branch: | bzr merge lp:~ajkavanagh/charm-helpers/fix-unit-paused-set-functions |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Liam Young (community) | Approve | ||
Review via email:
|
Commit message
Description of the change
Fixes broken *_unit_paused_set() functions in contrib/
Also fixes some PEP8 formatting in the utils.py and test_openstack_
To post a comment you must log in.
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'charmhelpers/contrib/openstack/utils.py' | |||
2 | --- charmhelpers/contrib/openstack/utils.py 2016-03-07 16:44:43 +0000 | |||
3 | +++ charmhelpers/contrib/openstack/utils.py 2016-03-08 12:40:42 +0000 | |||
4 | @@ -772,7 +772,8 @@ | |||
5 | 772 | os.mkdir(parent_dir) | 772 | os.mkdir(parent_dir) |
6 | 773 | 773 | ||
7 | 774 | juju_log('Cloning git repo: {}, branch: {}'.format(repo, branch)) | 774 | juju_log('Cloning git repo: {}, branch: {}'.format(repo, branch)) |
9 | 775 | repo_dir = install_remote(repo, dest=parent_dir, branch=branch, depth=depth) | 775 | repo_dir = install_remote( |
10 | 776 | repo, dest=parent_dir, branch=branch, depth=depth) | ||
11 | 776 | 777 | ||
12 | 777 | venv = os.path.join(parent_dir, 'venv') | 778 | venv = os.path.join(parent_dir, 'venv') |
13 | 778 | 779 | ||
14 | @@ -1016,8 +1017,8 @@ | |||
15 | 1016 | # Normal case relation ID exists but no related unit | 1017 | # Normal case relation ID exists but no related unit |
16 | 1017 | # (joining) | 1018 | # (joining) |
17 | 1018 | else: | 1019 | else: |
20 | 1019 | juju_log("{} relations's interface, {}, is related but has " | 1020 | juju_log("{} relations's interface, {}, is related but has" |
21 | 1020 | "no units in the relation." | 1021 | " no units in the relation." |
22 | 1021 | "".format(generic_interface, related_interface), | 1022 | "".format(generic_interface, related_interface), |
23 | 1022 | "INFO") | 1023 | "INFO") |
24 | 1023 | # Related unit exists and data missing on the relation | 1024 | # Related unit exists and data missing on the relation |
25 | @@ -1377,7 +1378,8 @@ | |||
26 | 1377 | """Set the unit to a paused state in the local kv() store. | 1378 | """Set the unit to a paused state in the local kv() store. |
27 | 1378 | This does NOT actually pause the unit | 1379 | This does NOT actually pause the unit |
28 | 1379 | """ | 1380 | """ |
30 | 1380 | with unitdata.HookData()() as kv: | 1381 | with unitdata.HookData()() as t: |
31 | 1382 | kv = t[0] | ||
32 | 1381 | kv.set('unit-paused', True) | 1383 | kv.set('unit-paused', True) |
33 | 1382 | 1384 | ||
34 | 1383 | 1385 | ||
35 | @@ -1386,7 +1388,8 @@ | |||
36 | 1386 | This does NOT actually restart any services - it only clears the | 1388 | This does NOT actually restart any services - it only clears the |
37 | 1387 | local state. | 1389 | local state. |
38 | 1388 | """ | 1390 | """ |
40 | 1389 | with unitdata.HookData()() as kv: | 1391 | with unitdata.HookData()() as t: |
41 | 1392 | kv = t[0] | ||
42 | 1390 | kv.set('unit-paused', False) | 1393 | kv.set('unit-paused', False) |
43 | 1391 | 1394 | ||
44 | 1392 | 1395 | ||
45 | @@ -1398,7 +1401,8 @@ | |||
46 | 1398 | if it excepts, return False | 1401 | if it excepts, return False |
47 | 1399 | """ | 1402 | """ |
48 | 1400 | try: | 1403 | try: |
50 | 1401 | with unitdata.HookData()() as kv: | 1404 | with unitdata.HookData()() as t: |
51 | 1405 | kv = t[0] | ||
52 | 1402 | # transform something truth-y into a Boolean. | 1406 | # transform something truth-y into a Boolean. |
53 | 1403 | return not(not(kv.get('unit-paused'))) | 1407 | return not(not(kv.get('unit-paused'))) |
54 | 1404 | except: | 1408 | except: |
55 | 1405 | 1409 | ||
56 | === modified file 'tests/contrib/openstack/test_openstack_utils.py' | |||
57 | --- tests/contrib/openstack/test_openstack_utils.py 2016-03-02 13:55:54 +0000 | |||
58 | +++ tests/contrib/openstack/test_openstack_utils.py 2016-03-08 12:40:42 +0000 | |||
59 | @@ -2,6 +2,7 @@ | |||
60 | 2 | import os | 2 | import os |
61 | 3 | import subprocess | 3 | import subprocess |
62 | 4 | import tempfile | 4 | import tempfile |
63 | 5 | import contextlib | ||
64 | 5 | import unittest | 6 | import unittest |
65 | 6 | from copy import copy | 7 | from copy import copy |
66 | 7 | from testtools import TestCase | 8 | from testtools import TestCase |
67 | @@ -196,8 +197,9 @@ | |||
68 | 196 | self.assertEquals(openstack.get_os_codename_install_source('distro'), | 197 | self.assertEquals(openstack.get_os_codename_install_source('distro'), |
69 | 197 | 'essex') | 198 | 'essex') |
70 | 198 | # proposed pocket | 199 | # proposed pocket |
73 | 199 | self.assertEquals(openstack.get_os_codename_install_source('distro-proposed'), | 200 | self.assertEquals(openstack.get_os_codename_install_source( |
74 | 200 | 'essex') | 201 | 'distro-proposed'), |
75 | 202 | 'essex') | ||
76 | 201 | 203 | ||
77 | 202 | # various cloud archive pockets | 204 | # various cloud archive pockets |
78 | 203 | src = 'cloud:precise-grizzly' | 205 | src = 'cloud:precise-grizzly' |
79 | @@ -777,7 +779,8 @@ | |||
80 | 777 | repository: 'git://git.openstack.org/openstack/requirements', | 779 | repository: 'git://git.openstack.org/openstack/requirements', |
81 | 778 | branch: stable/juno}""" | 780 | branch: stable/juno}""" |
82 | 779 | openstack.git_clone_and_install(git_wrong_order_1, 'keystone') | 781 | openstack.git_clone_and_install(git_wrong_order_1, 'keystone') |
84 | 780 | error_out.assert_called_with('keystone git repo must be specified last') | 782 | error_out.assert_called_with( |
85 | 783 | 'keystone git repo must be specified last') | ||
86 | 781 | 784 | ||
87 | 782 | git_wrong_order_2 = """ | 785 | git_wrong_order_2 = """ |
88 | 783 | repositories: | 786 | repositories: |
89 | @@ -785,7 +788,8 @@ | |||
90 | 785 | repository: 'git://git.openstack.org/openstack/keystone', | 788 | repository: 'git://git.openstack.org/openstack/keystone', |
91 | 786 | branch: stable/juno}""" | 789 | branch: stable/juno}""" |
92 | 787 | openstack.git_clone_and_install(git_wrong_order_2, 'keystone') | 790 | openstack.git_clone_and_install(git_wrong_order_2, 'keystone') |
94 | 788 | error_out.assert_called_with('requirements git repo must be specified first') | 791 | error_out.assert_called_with( |
95 | 792 | 'requirements git repo must be specified first') | ||
96 | 789 | 793 | ||
97 | 790 | @patch('os.path.join') | 794 | @patch('os.path.join') |
98 | 791 | @patch.object(openstack, 'charm_dir') | 795 | @patch.object(openstack, 'charm_dir') |
99 | @@ -839,8 +843,8 @@ | |||
100 | 839 | path_exists.return_value = False | 843 | path_exists.return_value = False |
101 | 840 | install_remote.return_value = dest_dir | 844 | install_remote.return_value = dest_dir |
102 | 841 | 845 | ||
105 | 842 | openstack._git_clone_and_install_single(repo, branch, depth, parent_dir, | 846 | openstack._git_clone_and_install_single( |
106 | 843 | http_proxy, False) | 847 | repo, branch, depth, parent_dir, http_proxy, False) |
107 | 844 | mkdir.assert_called_with(parent_dir) | 848 | mkdir.assert_called_with(parent_dir) |
108 | 845 | install_remote.assert_called_with(repo, dest=parent_dir, depth=1, | 849 | install_remote.assert_called_with(repo, dest=parent_dir, depth=1, |
109 | 846 | branch=branch) | 850 | branch=branch) |
110 | @@ -855,10 +859,9 @@ | |||
111 | 855 | @patch.object(openstack, 'install_remote') | 859 | @patch.object(openstack, 'install_remote') |
112 | 856 | @patch.object(openstack, 'pip_install') | 860 | @patch.object(openstack, 'pip_install') |
113 | 857 | @patch.object(openstack, '_git_update_requirements') | 861 | @patch.object(openstack, '_git_update_requirements') |
118 | 858 | def test_git_clone_and_install_single_with_update(self, _git_update_reqs, | 862 | def test_git_clone_and_install_single_with_update( |
119 | 859 | pip_install, | 863 | self, _git_update_reqs, pip_install, install_remote, log, |
120 | 860 | install_remote, log, | 864 | path_exists, mkdir, join): |
117 | 861 | path_exists, mkdir, join): | ||
121 | 862 | repo = 'git://git.openstack.org/openstack/requirements.git' | 865 | repo = 'git://git.openstack.org/openstack/requirements.git' |
122 | 863 | branch = 'master' | 866 | branch = 'master' |
123 | 864 | depth = 1 | 867 | depth = 1 |
124 | @@ -872,8 +875,8 @@ | |||
125 | 872 | path_exists.return_value = False | 875 | path_exists.return_value = False |
126 | 873 | install_remote.return_value = dest_dir | 876 | install_remote.return_value = dest_dir |
127 | 874 | 877 | ||
130 | 875 | openstack._git_clone_and_install_single(repo, branch, depth, parent_dir, | 878 | openstack._git_clone_and_install_single( |
131 | 876 | http_proxy, True) | 879 | repo, branch, depth, parent_dir, http_proxy, True) |
132 | 877 | mkdir.assert_called_with(parent_dir) | 880 | mkdir.assert_called_with(parent_dir) |
133 | 878 | install_remote.assert_called_with(repo, dest=parent_dir, depth=1, | 881 | install_remote.assert_called_with(repo, dest=parent_dir, depth=1, |
134 | 879 | branch=branch) | 882 | branch=branch) |
135 | @@ -914,7 +917,8 @@ | |||
136 | 914 | 'identity': ['identity-service']} | 917 | 'identity': ['identity-service']} |
137 | 915 | expected_result = 'identity' | 918 | expected_result = 'identity' |
138 | 916 | 919 | ||
140 | 917 | result = openstack.incomplete_relation_data(configs, required_interfaces) | 920 | result = openstack.incomplete_relation_data( |
141 | 921 | configs, required_interfaces) | ||
142 | 918 | self.assertTrue(expected_result in result.keys()) | 922 | self.assertTrue(expected_result in result.keys()) |
143 | 919 | 923 | ||
144 | 920 | @patch.object(openstack, 'juju_log') | 924 | @patch.object(openstack, 'juju_log') |
145 | @@ -1431,6 +1435,43 @@ | |||
146 | 1431 | 'Services should be paused but these ports ' | 1435 | 'Services should be paused but these ports ' |
147 | 1432 | 'which should be closed, but are open: 60') | 1436 | 'which should be closed, but are open: 60') |
148 | 1433 | 1437 | ||
149 | 1438 | @staticmethod | ||
150 | 1439 | def _unit_paused_helper(hook_data_mock): | ||
151 | 1440 | # HookData()() returns a tuple (kv, delta_config, delta_relation) | ||
152 | 1441 | # but we only want kv in the test. | ||
153 | 1442 | kv = MagicMock() | ||
154 | 1443 | |||
155 | 1444 | @contextlib.contextmanager | ||
156 | 1445 | def hook_data__call__(): | ||
157 | 1446 | yield (kv, True, False) | ||
158 | 1447 | |||
159 | 1448 | hook_data__call__.return_value = (kv, True, False) | ||
160 | 1449 | hook_data_mock.return_value = hook_data__call__ | ||
161 | 1450 | return kv | ||
162 | 1451 | |||
163 | 1452 | @patch('charmhelpers.contrib.openstack.utils.unitdata.HookData') | ||
164 | 1453 | def test_set_unit_paused(self, hook_data): | ||
165 | 1454 | kv = self._unit_paused_helper(hook_data) | ||
166 | 1455 | openstack.set_unit_paused() | ||
167 | 1456 | kv.set.assert_called_once_with('unit-paused', True) | ||
168 | 1457 | |||
169 | 1458 | @patch('charmhelpers.contrib.openstack.utils.unitdata.HookData') | ||
170 | 1459 | def test_clear_unit_paused(self, hook_data): | ||
171 | 1460 | kv = self._unit_paused_helper(hook_data) | ||
172 | 1461 | openstack.clear_unit_paused() | ||
173 | 1462 | kv.set.assert_called_once_with('unit-paused', False) | ||
174 | 1463 | |||
175 | 1464 | @patch('charmhelpers.contrib.openstack.utils.unitdata.HookData') | ||
176 | 1465 | def test_is_unit_paused_set(self, hook_data): | ||
177 | 1466 | kv = self._unit_paused_helper(hook_data) | ||
178 | 1467 | kv.get.return_value = True | ||
179 | 1468 | r = openstack.is_unit_paused_set() | ||
180 | 1469 | kv.get.assert_called_once_with('unit-paused') | ||
181 | 1470 | self.assertEquals(r, True) | ||
182 | 1471 | kv.get.return_value = False | ||
183 | 1472 | r = openstack.is_unit_paused_set() | ||
184 | 1473 | self.assertEquals(r, False) | ||
185 | 1474 | |||
186 | 1434 | @patch('charmhelpers.contrib.openstack.utils.service_pause') | 1475 | @patch('charmhelpers.contrib.openstack.utils.service_pause') |
187 | 1435 | @patch('charmhelpers.contrib.openstack.utils.set_unit_paused') | 1476 | @patch('charmhelpers.contrib.openstack.utils.set_unit_paused') |
188 | 1436 | def test_pause_unit_okay(self, set_unit_paused, service_pause): | 1477 | def test_pause_unit_okay(self, set_unit_paused, service_pause): |
189 | @@ -1507,7 +1548,8 @@ | |||
190 | 1507 | 1548 | ||
191 | 1508 | @patch('charmhelpers.contrib.openstack.utils.service_resume') | 1549 | @patch('charmhelpers.contrib.openstack.utils.service_resume') |
192 | 1509 | @patch('charmhelpers.contrib.openstack.utils.clear_unit_paused') | 1550 | @patch('charmhelpers.contrib.openstack.utils.clear_unit_paused') |
194 | 1510 | def test_resume_unit_service_fails(self, clear_unit_paused, service_resume): | 1551 | def test_resume_unit_service_fails( |
195 | 1552 | self, clear_unit_paused, service_resume): | ||
196 | 1511 | services = ['service1', 'service2'] | 1553 | services = ['service1', 'service2'] |
197 | 1512 | service_resume.side_effect = [True, True] | 1554 | service_resume.side_effect = [True, True] |
198 | 1513 | openstack.resume_unit(None, services=services) | 1555 | openstack.resume_unit(None, services=services) |
199 | @@ -1519,8 +1561,8 @@ | |||
200 | 1519 | openstack.resume_unit(None, services=services) | 1561 | openstack.resume_unit(None, services=services) |
201 | 1520 | raise Exception("resume_unit should have raised Exception") | 1562 | raise Exception("resume_unit should have raised Exception") |
202 | 1521 | except Exception as e: | 1563 | except Exception as e: |
205 | 1522 | self.assertEquals(e.args[0], | 1564 | self.assertEquals( |
206 | 1523 | "Couldn't resume: service2 didn't start cleanly.") | 1565 | e.args[0], "Couldn't resume: service2 didn't start cleanly.") |
207 | 1524 | 1566 | ||
208 | 1525 | @patch('charmhelpers.contrib.openstack.utils.service_resume') | 1567 | @patch('charmhelpers.contrib.openstack.utils.service_resume') |
209 | 1526 | @patch('charmhelpers.contrib.openstack.utils.clear_unit_paused') | 1568 | @patch('charmhelpers.contrib.openstack.utils.clear_unit_paused') |
LGTM, thanks for the fixes