Merge ~hloeung/jenkins-agent-charm:cleanup into jenkins-agent-charm:master
- Git
- lp:~hloeung/jenkins-agent-charm
- cleanup
- Merge into master
Proposed by
Haw Loeung
Status: | Merged |
---|---|
Approved by: | Haw Loeung |
Approved revision: | b6ad34249106d0e47d23a08e1571378a44ffd973 |
Merged at revision: | 63a4a4bf7be28b85f2d972b06339c498cc703082 |
Proposed branch: | ~hloeung/jenkins-agent-charm:cleanup |
Merge into: | jenkins-agent-charm:master |
Prerequisite: | ~hloeung/jenkins-agent-charm:master |
Diff against target: |
276 lines (+77/-53) 4 files modified
Makefile (+4/-0) reactive/jenkins_slave.py (+11/-13) tests/unit/test_jenkins_slave.py (+58/-40) tox.ini (+4/-0) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Stuart Bishop (community) | Approve | ||
Canonical IS Reviewers | Pending | ||
Review via email: mp+371238@code.launchpad.net |
Commit message
Add and normalise python layout with black
Description of the change
To post a comment you must log in.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Unable to determine commit message from repository - please click "Set commit message" and enter the commit message manually.
Revision history for this message
🤖 Canonical IS Merge Bot (canonical-is-mergebot) wrote : | # |
Change successfully merged at revision 63a4a4bf7be28b8
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/Makefile b/Makefile |
2 | index 43470ed..c0b3852 100644 |
3 | --- a/Makefile |
4 | +++ b/Makefile |
5 | @@ -10,6 +10,8 @@ help: |
6 | @echo "" |
7 | |
8 | lint: |
9 | + @echo "Normalising python layout with black." |
10 | + @tox -e black |
11 | @echo "Running flake8" |
12 | @tox -e lint |
13 | |
14 | @@ -25,6 +27,8 @@ clean: |
15 | @echo "Cleaning files" |
16 | @rm -rf ./.tox |
17 | @rm -rf ./.pytest_cache |
18 | + @rm -rf ./tests/unit/__pycache__ ./reactive/__pycache__ ./lib/__pycache__ |
19 | + @rm -rf ./.coverage ./.unit-state.db |
20 | |
21 | # The targets below don't depend on a file |
22 | .PHONY: lint test unittest functionaltest clean help |
23 | diff --git a/reactive/jenkins_slave.py b/reactive/jenkins_slave.py |
24 | index 29bf58f..9c2089b 100755 |
25 | --- a/reactive/jenkins_slave.py |
26 | +++ b/reactive/jenkins_slave.py |
27 | @@ -215,20 +215,18 @@ def file_to_units(local_path, unit_path, perms=None, owner='root', group='root') |
28 | file_perms = 0o644 |
29 | |
30 | with open(local_path, 'r') as fh: |
31 | - host.write_file( |
32 | - path=unit_path, |
33 | - content=fh.read().encode(), |
34 | - owner=owner, |
35 | - group=group, |
36 | - perms=file_perms, |
37 | - ) |
38 | + host.write_file(path=unit_path, content=fh.read().encode(), owner=owner, group=group, perms=file_perms) |
39 | |
40 | |
41 | -def write_default_conf(master_url=None, owner='root', group='root', |
42 | - conf_path='/etc/default/jenkins-slave'): |
43 | +def write_default_conf(master_url=None, owner='root', group='root', conf_path='/etc/default/jenkins-slave'): |
44 | templates_dir = os.path.join(hookenv.charm_dir(), 'templates') |
45 | slave_host = hookenv.local_unit().replace('/', '-') |
46 | - templating.render('jenkins-slave-default', conf_path, |
47 | - {'master_url': master_url, 'slave_host': slave_host}, |
48 | - owner=owner, group=group, perms=0o444, |
49 | - templates_dir=templates_dir) |
50 | + templating.render( |
51 | + 'jenkins-slave-default', |
52 | + conf_path, |
53 | + {'master_url': master_url, 'slave_host': slave_host}, |
54 | + owner=owner, |
55 | + group=group, |
56 | + perms=0o444, |
57 | + templates_dir=templates_dir, |
58 | + ) |
59 | diff --git a/tests/unit/test_jenkins_slave.py b/tests/unit/test_jenkins_slave.py |
60 | index 535bc95..b33f1ed 100644 |
61 | --- a/tests/unit/test_jenkins_slave.py |
62 | +++ b/tests/unit/test_jenkins_slave.py |
63 | @@ -71,8 +71,13 @@ class TestSetDefaultConf(unittest.TestCase): |
64 | patcher = mock.patch('os.uname') |
65 | self.mock_log = patcher.start() |
66 | self.addCleanup(patcher.stop) |
67 | - self.mock_log.return_value = ('Linux', 'mock-jenkins-slave', '4.15.0-46-generic', |
68 | - '#49-Ubuntu SMP Wed Feb 6 09:33:07 UTC 2019', 'x86_64') |
69 | + self.mock_log.return_value = ( |
70 | + 'Linux', |
71 | + 'mock-jenkins-slave', |
72 | + '4.15.0-46-generic', |
73 | + '#49-Ubuntu SMP Wed Feb 6 09:33:07 UTC 2019', |
74 | + 'x86_64', |
75 | + ) |
76 | |
77 | patcher = mock.patch('os.cpu_count') |
78 | self.mock_log = patcher.start() |
79 | @@ -89,9 +94,11 @@ class TestSetDefaultConf(unittest.TestCase): |
80 | '''Test correct flags set via upgrade-charm hook''' |
81 | jenkins_slave.upgrade_charm() |
82 | self.assertFalse(status.maintenance.assert_called()) |
83 | - expected = [mock.call('jenkins-slave.active'), |
84 | - mock.call('jenkins-slave.installed'), |
85 | - mock.call('nagios-nrpe.configured')] |
86 | + expected = [ |
87 | + mock.call('jenkins-slave.active'), |
88 | + mock.call('jenkins-slave.installed'), |
89 | + mock.call('nagios-nrpe.configured'), |
90 | + ] |
91 | self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True)) |
92 | |
93 | @mock.patch('charms.reactive.clear_flag') |
94 | @@ -99,8 +106,7 @@ class TestSetDefaultConf(unittest.TestCase): |
95 | '''Test correct flags are set via config-changed charm hook''' |
96 | jenkins_slave.config_changed() |
97 | self.assertFalse(status.maintenance.assert_called()) |
98 | - expected = [mock.call('jenkins-slave.configured'), |
99 | - mock.call('nagios-nrpe.configured')] |
100 | + expected = [mock.call('jenkins-slave.configured'), mock.call('nagios-nrpe.configured')] |
101 | self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True)) |
102 | |
103 | @mock.patch('charmhelpers.core.hookenv.config') |
104 | @@ -112,8 +118,8 @@ class TestSetDefaultConf(unittest.TestCase): |
105 | @mock.patch('reactive.jenkins_slave.file_to_units') |
106 | @mock.patch('reactive.jenkins_slave.write_default_conf') |
107 | def test_hook_install( |
108 | - self, write_default_conf, file_to_units, apt_purge, user_exists, service, |
109 | - mkdir, adduser, config): |
110 | + self, write_default_conf, file_to_units, apt_purge, user_exists, service, mkdir, adduser, config |
111 | + ): |
112 | config.return_value = {'tools': 'some-tools-package'} |
113 | apt.install_queued.return_value = True |
114 | user_exists.return_value = False |
115 | @@ -122,18 +128,24 @@ class TestSetDefaultConf(unittest.TestCase): |
116 | |
117 | expected = [mock.call(home_dir='/var/lib/jenkins', system_user=True, username='jenkins')] |
118 | self.assertFalse(adduser.assert_has_calls(expected, any_order=True)) |
119 | - expected = [mock.call('/var/lib/jenkins', group='jenkins', owner='jenkins'), |
120 | - mock.call('/var/log/jenkins', group='jenkins', owner='jenkins')] |
121 | + expected = [ |
122 | + mock.call('/var/lib/jenkins', group='jenkins', owner='jenkins'), |
123 | + mock.call('/var/log/jenkins', group='jenkins', owner='jenkins'), |
124 | + ] |
125 | self.assertFalse(mkdir.assert_has_calls(expected, any_order=True)) |
126 | self.assertFalse(service.assert_has_calls([mock.call('enable', 'jenkins-slave')], any_order=True)) |
127 | self.assertFalse(apt_purge.assert_has_calls([mock.call(['jenkins-slave'])], any_order=True)) |
128 | - expected = [mock.call('files/download-slave.sh', '/usr/local/sbin/download-slave.sh'), |
129 | - mock.call('files/jenkins-slave-logrotate-config', '/etc/logrotate.d/jenkins-slave'), |
130 | - mock.call('files/jenkins-slave-systemd-config', '/lib/systemd/system/jenkins-slave.service')] |
131 | + expected = [ |
132 | + mock.call('files/download-slave.sh', '/usr/local/sbin/download-slave.sh'), |
133 | + mock.call('files/jenkins-slave-logrotate-config', '/etc/logrotate.d/jenkins-slave'), |
134 | + mock.call('files/jenkins-slave-systemd-config', '/lib/systemd/system/jenkins-slave.service'), |
135 | + ] |
136 | self.assertFalse(file_to_units.assert_has_calls(expected, any_order=True)) |
137 | self.assertFalse(write_default_conf.assert_called()) |
138 | - expected = [mock.call.queue_install(['wget', 'default-jre-headless', 'some-tools-package']), |
139 | - mock.call.install_queued()] |
140 | + expected = [ |
141 | + mock.call.queue_install(['wget', 'default-jre-headless', 'some-tools-package']), |
142 | + mock.call.install_queued(), |
143 | + ] |
144 | self.assertEqual(apt.method_calls, expected) |
145 | |
146 | apt.install_queued.return_value = False |
147 | @@ -168,8 +180,9 @@ class TestSetDefaultConf(unittest.TestCase): |
148 | @mock.patch('charmhelpers.core.unitdata.kv') |
149 | @mock.patch('reactive.jenkins_slave.write_default_conf') |
150 | @mock.patch('reactive.jenkins_slave.file_to_units') |
151 | - def test_configure_jenkins_slave_no_url(self, file_to_units, write_default_conf, unitdata_kv, config, |
152 | - set_flag, clear_flag): |
153 | + def test_configure_jenkins_slave_no_url( |
154 | + self, file_to_units, write_default_conf, unitdata_kv, config, set_flag, clear_flag |
155 | + ): |
156 | config.return_value = {} |
157 | unitdata_kv.return_value = {} |
158 | jenkins_slave.configure_jenkins_slave() |
159 | @@ -183,8 +196,9 @@ class TestSetDefaultConf(unittest.TestCase): |
160 | @mock.patch('charmhelpers.core.unitdata.kv') |
161 | @mock.patch('reactive.jenkins_slave.write_default_conf') |
162 | @mock.patch('reactive.jenkins_slave.file_to_units') |
163 | - def test_configure_jenkins_slave_master_url(self, file_to_units, write_default_conf, unitdata_kv, config, |
164 | - set_flag, clear_flag): |
165 | + def test_configure_jenkins_slave_master_url( |
166 | + self, file_to_units, write_default_conf, unitdata_kv, config, set_flag, clear_flag |
167 | + ): |
168 | config.return_value = {'master_url': 'http://10.1.1.1:8080'} |
169 | unitdata_kv.return_value = {} |
170 | jenkins_slave.configure_jenkins_slave() |
171 | @@ -199,8 +213,9 @@ class TestSetDefaultConf(unittest.TestCase): |
172 | @mock.patch('charmhelpers.core.unitdata.kv') |
173 | @mock.patch('reactive.jenkins_slave.write_default_conf') |
174 | @mock.patch('reactive.jenkins_slave.file_to_units') |
175 | - def test_configure_jenkins_slave_relation_url(self, file_to_units, write_default_conf, unitdata_kv, config, |
176 | - set_flag, clear_flag): |
177 | + def test_configure_jenkins_slave_relation_url( |
178 | + self, file_to_units, write_default_conf, unitdata_kv, config, set_flag, clear_flag |
179 | + ): |
180 | config.return_value = {} |
181 | unitdata_kv.return_value = {'url': 'http://10.22.22.22:8080'} |
182 | jenkins_slave.configure_jenkins_slave() |
183 | @@ -253,10 +268,7 @@ class TestSetDefaultConf(unittest.TestCase): |
184 | @mock.patch('charms.reactive.set_flag') |
185 | def test_hook_slave_relation_changed_flags(self, set_flag, clear_flag): |
186 | jenkins_slave.slave_relation_changed() |
187 | - expected = [ |
188 | - mock.call('jenkins-slave.configured'), |
189 | - mock.call('slave-relation.configured'), |
190 | - ] |
191 | + expected = [mock.call('jenkins-slave.configured'), mock.call('slave-relation.configured')] |
192 | self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True)) |
193 | self.assertFalse(set_flag.assert_has_calls([mock.call('slave-relation.available')], any_order=True)) |
194 | |
195 | @@ -273,28 +285,32 @@ class TestSetDefaultConf(unittest.TestCase): |
196 | @mock.patch('charmhelpers.core.unitdata.kv') |
197 | @mock.patch('reactive.jenkins_slave.write_default_conf') |
198 | def test_hook_slave_relation( |
199 | - self, write_default_conf, unitdata_kv, relation_set, relation_get, |
200 | - config, set_flag, clear_flag): |
201 | + self, write_default_conf, unitdata_kv, relation_set, relation_get, config, set_flag, clear_flag |
202 | + ): |
203 | config.return_value = {'master_url': ''} |
204 | relation_get.return_value = 'http://10.1.1.1:8080' |
205 | jenkins_slave.slave_relation() |
206 | self.assertFalse(relation_get.assert_called_once_with('url')) |
207 | self.assertFalse(clear_flag.assert_called_once_with('jenkins-slave.active')) |
208 | self.assertFalse(set_flag.assert_called_once_with('slave-relation.configured')) |
209 | - expected = [mock.call(executors=3), |
210 | - mock.call(labels='x86_64'), |
211 | - mock.call(slavehost='mock-jenkins-slave-0'), |
212 | - mock.call(slaveaddress='10.1.2.3')] |
213 | + expected = [ |
214 | + mock.call(executors=3), |
215 | + mock.call(labels='x86_64'), |
216 | + mock.call(slavehost='mock-jenkins-slave-0'), |
217 | + mock.call(slaveaddress='10.1.2.3'), |
218 | + ] |
219 | self.assertFalse(relation_set.assert_has_calls(expected, any_order=True)) |
220 | |
221 | config.return_value = {'master_url': '', 'labels': 'label1 label2'} |
222 | relation_get.return_value = 'http://10.1.1.1:8080' |
223 | relation_set.reset_mock() |
224 | jenkins_slave.slave_relation() |
225 | - expected = [mock.call(executors=3), |
226 | - mock.call(labels='label1 label2'), |
227 | - mock.call(slavehost='mock-jenkins-slave-0'), |
228 | - mock.call(slaveaddress='10.1.2.3')] |
229 | + expected = [ |
230 | + mock.call(executors=3), |
231 | + mock.call(labels='label1 label2'), |
232 | + mock.call(slavehost='mock-jenkins-slave-0'), |
233 | + mock.call(slaveaddress='10.1.2.3'), |
234 | + ] |
235 | self.assertFalse(relation_set.assert_has_calls(expected, any_order=True)) |
236 | |
237 | @mock.patch('charmhelpers.core.hookenv.config') |
238 | @@ -311,8 +327,7 @@ class TestSetDefaultConf(unittest.TestCase): |
239 | @mock.patch('charms.reactive.set_flag') |
240 | @mock.patch('charmhelpers.core.hookenv.config') |
241 | @mock.patch('charmhelpers.core.hookenv.relation_get') |
242 | - def test_hook_slave_relation_relation_url_not_set( |
243 | - self, relation_get, config, set_flag, clear_flag): |
244 | + def test_hook_slave_relation_relation_url_not_set(self, relation_get, config, set_flag, clear_flag): |
245 | config.return_value = {'master_url': ''} |
246 | relation_get.return_value = '' |
247 | jenkins_slave.slave_relation() |
248 | @@ -326,8 +341,11 @@ class TestSetDefaultConf(unittest.TestCase): |
249 | nrpe_instance_mock = nrpe(get_nagios_hostname(), primary=True) |
250 | jenkins_slave.configure_nagios(None) |
251 | status.maintenance.assert_called() |
252 | - want = [mock.call('jenkins_slave_ps', 'Jenkins Slave Process', |
253 | - '/usr/lib/nagios/plugins/check_procs -c 1:1 -a slave.jar')] |
254 | + want = [ |
255 | + mock.call( |
256 | + 'jenkins_slave_ps', 'Jenkins Slave Process', '/usr/lib/nagios/plugins/check_procs -c 1:1 -a slave.jar' |
257 | + ) |
258 | + ] |
259 | nrpe_instance_mock.add_check.assert_has_calls(want, any_order=True) |
260 | |
261 | nrpe_instance_mock.write.assert_called() |
262 | diff --git a/tox.ini b/tox.ini |
263 | index 99078b2..220ff5d 100644 |
264 | --- a/tox.ini |
265 | +++ b/tox.ini |
266 | @@ -20,6 +20,10 @@ basepython = python3 |
267 | commands = functest-run-suite --keep-model |
268 | deps = -r{toxinidir}/tests/functional/requirements.txt |
269 | |
270 | +[testenv:black] |
271 | +commands = black --skip-string-normalization --line-length=120 . |
272 | +deps = black |
273 | + |
274 | [testenv:lint] |
275 | commands = flake8 |
276 | deps = flake8 |
This merge proposal is being monitored by mergebot. Change the status to Approved to merge.