Merge lp:~bigkevmcd/ubuntu-system-image/some-tidyups into lp:ubuntu-system-image/server

Proposed by Kevin McDermott on 2013-10-01
Status: Rejected
Rejected by: Stéphane Graber on 2014-03-05
Proposed branch: lp:~bigkevmcd/ubuntu-system-image/some-tidyups
Merge into: lp:ubuntu-system-image/server
Diff against target: 516 lines (+224/-111)
9 files modified
bin/copy-image (+1/-1)
bin/generate-keyrings (+1/-1)
bin/import-images (+1/-1)
lib/systemimage/config.py (+4/-0)
tests/generate-keys (+1/-1)
tests/run (+1/-1)
tests/test_config.py (+206/-99)
tests/test_static.py (+8/-6)
utils/check-latest (+1/-1)
To merge this branch: bzr merge lp:~bigkevmcd/ubuntu-system-image/some-tidyups
Reviewer Review Type Date Requested Status
Stéphane Graber 2013-10-01 Disapprove on 2014-03-05
Review via email: mp+188651@code.launchpad.net

Description of the change

This is just a quick tidyup as part of preparing to take on maintenance.

I split out all the config tests, and removed hard-coded Python references.

To post a comment you must log in.
Stéphane Graber (stgraber) wrote :

The use of /usr/bin/python and /usr/bin/python3 is deliberate and "/usr/bin/env pythonX" is considered a bad upstream practice.

I'm also a bit confused by your "as part of preparing to take on maintenance" since I'm the maintainer for that code :)

I see quite a few good things in there though:
 - consistent use of find_on_path
 - extra comments

The rest as far as I can tell (though it's hard to catch everything) seems mostly to be about splitting the tests even more which I'd be opposed to. Granularity is good but it can be sufficiently achieved by using the self.assert* functions. Creating separate test functions for all the various cases will create a ton of code duplication and cause the setup/teardown functions to trigger a lot more often (and those are pretty costly).

I'm going to reject this merge proposal for now. I'd really appreciate it if you could come up with separate merge proposals for each chunk of changes so they can be properly reviewed and discussed.

Thanks

review: Disapprove

Unmerged revisions

167. By Kevin McDermott on 2013-10-01

Some tidyups.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bin/copy-image'
2--- bin/copy-image 2013-09-15 22:56:21 +0000
3+++ bin/copy-image 2013-10-01 16:35:07 +0000
4@@ -1,4 +1,4 @@
5-#!/usr/bin/python
6+#!/usr/bin/env python
7 # -*- coding: utf-8 -*-
8
9 # Copyright (C) 2013 Canonical Ltd.
10
11=== modified file 'bin/generate-keyrings'
12--- bin/generate-keyrings 2013-07-02 21:04:10 +0000
13+++ bin/generate-keyrings 2013-10-01 16:35:07 +0000
14@@ -1,4 +1,4 @@
15-#!/usr/bin/python
16+#!/usr/bin/env python
17 # -*- coding: utf-8 -*-
18
19 # Copyright (C) 2013 Canonical Ltd.
20
21=== modified file 'bin/import-images'
22--- bin/import-images 2013-09-16 14:51:06 +0000
23+++ bin/import-images 2013-10-01 16:35:07 +0000
24@@ -1,4 +1,4 @@
25-#!/usr/bin/python
26+#!/usr/bin/env python
27 # -*- coding: utf-8 -*-
28
29 # Copyright (C) 2013 Canonical Ltd.
30
31=== modified file 'lib/systemimage/config.py'
32--- lib/systemimage/config.py 2013-09-16 16:49:27 +0000
33+++ lib/systemimage/config.py 2013-10-01 16:35:07 +0000
34@@ -24,6 +24,10 @@
35
36
37 def parse_config(path):
38+ """
39+ Parses a ConfigParser style file and the converts it to a dictionary with a
40+ mapping of sections to dictionaries of key/value mappings.
41+ """
42 config = {}
43
44 configp = ConfigParser()
45
46=== modified file 'tests/generate-keys'
47--- tests/generate-keys 2013-07-02 21:04:10 +0000
48+++ tests/generate-keys 2013-10-01 16:35:07 +0000
49@@ -1,4 +1,4 @@
50-#!/usr/bin/python
51+#!/usr/bin/env python
52 # -*- coding: utf-8 -*-
53
54 # Copyright (C) 2013 Canonical Ltd.
55
56=== modified file 'tests/run'
57--- tests/run 2013-07-02 21:04:10 +0000
58+++ tests/run 2013-10-01 16:35:07 +0000
59@@ -1,4 +1,4 @@
60-#!/usr/bin/python
61+#!/usr/bin/env python
62 # -*- coding: utf-8 -*-
63
64 # Copyright (C) 2013 Canonical Ltd.
65
66=== modified file 'tests/test_config.py'
67--- tests/test_config.py 2013-09-16 23:20:47 +0000
68+++ tests/test_config.py 2013-10-01 16:35:07 +0000
69@@ -37,12 +37,13 @@
70 def tearDown(self):
71 shutil.rmtree(self.temp_directory)
72
73- @mock.patch("subprocess.call")
74- def test_config(self, mock_call):
75- # Good complete config
76+ def test_parse_config(self):
77+ """
78+ parse_config should return a dictionary with keys for each section in
79+ the ConfigParser style file, and each value should be key/value for the
80+ keys within the section.
81+ """
82 config_path = os.path.join(self.temp_directory, "config")
83- key_path = os.path.join(self.temp_directory, "key")
84-
85 with open(config_path, "w+") as fd:
86 fd.write("""[global]
87 base_path = %s
88@@ -53,36 +54,53 @@
89 ssh_key = key
90 ssh_port = 22
91 ssh_command = command
92-
93-[mirror_a]
94-ssh_host = hosta
95-
96-[mirror_b]
97-ssh_host = hostb
98 """ % self.temp_directory)
99-
100- conf = config.Config(config_path)
101-
102- # Test ssh sync
103- tools.sync_mirrors(conf)
104- expected_calls = [((['ssh', '-i', key_path, '-l', 'user',
105- '-p', '22', 'hosta', 'command'],), {}),
106- ((['ssh', '-i', key_path, '-l', 'user',
107- '-p', '22', 'hostb', 'command'],), {})]
108- self.assertEquals(mock_call.call_args_list, expected_calls)
109-
110+ parsed_config = config.parse_config(config_path)
111+ self.assertEqual(
112+ {"global": {
113+ "base_path": self.temp_directory,
114+ "mirrors": ["a", "b"]},
115+ "mirror_default": {
116+ "ssh_command": "command",
117+ "ssh_key": "key",
118+ "ssh_port": "22",
119+ "ssh_user": "user"
120+ }
121+ },
122+ parsed_config)
123+
124+ def test_parse_config_with_invalid_config(self):
125+ """
126+ parse_config should return an empty dictionary for invalid config
127+ files.
128+ """
129 # Invalid config
130 invalid_config_path = os.path.join(self.temp_directory,
131 "invalid_config")
132 with open(invalid_config_path, "w+") as fd:
133 fd.write("""invalid""")
134
135- self.assertEquals(config.parse_config(invalid_config_path), {})
136-
137- self.assertRaises(
138- Exception, config.Config, os.path.join(self.temp_directory,
139- "invalid"))
140-
141+ self.assertEqual(config.parse_config(invalid_config_path), {})
142+
143+ def test_instantiating_config_with_missing_file(self):
144+ """
145+ We should get an error if we pass a missing file to the Config
146+ constructor.
147+ """
148+ with self.assertRaises(Exception) as cm:
149+ config.Config()
150+ # Unfortunately we err with the alternative path, rather than the one
151+ # supplied.
152+ alternative_path = os.path.realpath(os.path.join(
153+ os.path.dirname(__file__), "../etc/config"))
154+ self.assertEqual(
155+ "Configuration file doesn't exist: %s" % alternative_path,
156+ str(cm.exception))
157+
158+ def test_instantiating_config_with_empty_file(self):
159+ """
160+ We can instantiate a Config with an empty file.
161+ """
162 # Empty config
163 empty_config_path = os.path.join(self.temp_directory,
164 "empty_config")
165@@ -90,8 +108,56 @@
166 fd.write("")
167
168 conf = config.Config(empty_config_path)
169- self.assertEquals(conf.base_path, os.getcwd())
170-
171+ self.assertEqual(os.environ.get("SYSTEM_IMAGE_ROOT"), conf.base_path)
172+
173+ def test_instantiating_config_with_base_path_in_config(self):
174+ """
175+ If the SYSTEM_IMAGE_ROOT environment variable is set, then the path
176+ should be used as the base_path.
177+ """
178+ test_path = os.path.join(self.temp_directory, "a", "b")
179+ os.makedirs(os.path.join(test_path, "etc"))
180+ with open(os.path.join(test_path, "etc", "config"), "w+") as fd:
181+ fd.write("[global]\nbase_path = a/b/c")
182+ os.environ['SYSTEM_IMAGE_ROOT'] = test_path
183+ test_config = config.Config()
184+ self.assertEqual(test_config.base_path, "a/b/c")
185+
186+ def test_instantiating_config_picks_up_basepath_from_config(self):
187+ """
188+ The base_path for the Config should be picked up from the
189+ configuration.
190+ """
191+ config_path = os.path.join(self.temp_directory, "config")
192+ with open(config_path, "w+") as fd:
193+ fd.write("[global]\nbase_path=%s\n" % self.temp_directory)
194+
195+ conf = config.Config(config_path)
196+ self.assertEqual(self.temp_directory, conf.base_path)
197+
198+ def test_config_with_missing_mirror_default(self):
199+ """
200+ Having a missing mirror_default section should cause an error.
201+ """
202+ missing_default_config_path = os.path.join(self.temp_directory,
203+ "missing_default_config")
204+ with open(missing_default_config_path, "w+") as fd:
205+ fd.write("""[global]
206+mirrors = a
207+
208+[mirror_a]
209+ssh_host = host
210+""")
211+ with self.assertRaises(KeyError) as cm:
212+ config.Config(missing_default_config_path)
213+ self.assertEqual(
214+ "'Missing mirror_default section.'",
215+ str(cm.exception))
216+
217+ def test_instantiating_config_with_single_mirror(self):
218+ """
219+ We can instantiate a Config with valid file with a single mirror.
220+ """
221 # Single mirror config
222 single_mirror_config_path = os.path.join(self.temp_directory,
223 "single_mirror_config")
224@@ -110,22 +176,13 @@
225 """)
226
227 conf = config.Config(single_mirror_config_path)
228- self.assertEquals(conf.mirrors['a'].ssh_command, "command")
229-
230- # Missing mirror_default
231- missing_default_config_path = os.path.join(self.temp_directory,
232- "missing_default_config")
233- with open(missing_default_config_path, "w+") as fd:
234- fd.write("""[global]
235-mirrors = a
236-
237-[mirror_a]
238-ssh_host = host
239-""")
240-
241- self.assertRaises(KeyError, config.Config, missing_default_config_path)
242-
243- # Missing mirror key
244+ self.assertEqual(conf.mirrors['a'].ssh_command, "command")
245+
246+ def test_missing_mirror_ssh_key(self):
247+ """
248+ The config should throw an exception if there's a missing ssh_key in a
249+ mirror definition.
250+ """
251 missing_key_config_path = os.path.join(self.temp_directory,
252 "missing_key_config")
253 with open(missing_key_config_path, "w+") as fd:
254@@ -140,26 +197,17 @@
255 [mirror_a]
256 ssh_host = host
257 """)
258-
259- self.assertRaises(KeyError, config.Config, missing_key_config_path)
260-
261- # Missing mirror
262- missing_mirror_config_path = os.path.join(self.temp_directory,
263- "missing_mirror_config")
264- with open(missing_mirror_config_path, "w+") as fd:
265- fd.write("""[global]
266-mirrors = a
267-
268-[mirror_default]
269-ssh_user = user
270-ssh_port = 22
271-ssh_command = command
272-ssh_key = key
273-""")
274-
275- self.assertRaises(KeyError, config.Config, missing_mirror_config_path)
276-
277- # Missing ssh_host
278+ with self.assertRaises(KeyError) as cm:
279+ config.Config(missing_key_config_path)
280+ self.assertEqual(
281+ "'Missing key in mirror_default: ssh_key'",
282+ str(cm.exception))
283+
284+ def test_missing_ssh_host_in_mirror(self):
285+ """
286+ We should get an appropriate error if there's a missing ssh_host in a
287+ mirror section.
288+ """
289 missing_host_config_path = os.path.join(self.temp_directory,
290 "missing_host_config")
291 with open(missing_host_config_path, "w+") as fd:
292@@ -178,17 +226,10 @@
293
294 self.assertRaises(KeyError, config.Config, missing_host_config_path)
295
296- # Test with env path
297- test_path = os.path.join(self.temp_directory, "a", "b")
298- os.makedirs(os.path.join(test_path, "etc"))
299- with open(os.path.join(test_path, "etc", "config"), "w+") as fd:
300- fd.write("[global]\nbase_path = a/b/c")
301- os.environ['SYSTEM_IMAGE_ROOT'] = test_path
302- test_config = config.Config()
303- self.assertEquals(test_config.base_path, "a/b/c")
304-
305- # Test the channels config
306- ## Multiple channels
307+ def test_multiple_channels_in_configuration(self):
308+ """
309+ Each of the channels should be parsed from the configuration.
310+ """
311 channel_config_path = os.path.join(self.temp_directory,
312 "channel_config")
313 with open(channel_config_path, "w+") as fd:
314@@ -209,24 +250,49 @@
315 """)
316
317 conf = config.Config(channel_config_path)
318- self.assertEquals(
319+ self.assertEqual(
320 conf.channels['b'].files,
321 [{'name': 'a', 'generator': 'test',
322 'arguments': ['arg1', 'arg2']},
323 {'name': 'b', 'generator': 'test',
324 'arguments': ['arg3', 'arg4']}])
325
326- self.assertEquals(conf.channels['a'].fullcount, 10)
327- self.assertEquals(conf.channels['a'].versionbase, 1)
328- self.assertEquals(conf.channels['a'].deltabase, ['a'])
329-
330- self.assertEquals(conf.channels['b'].fullcount, 0)
331- self.assertEquals(conf.channels['b'].versionbase, 5)
332- self.assertEquals(conf.channels['b'].deltabase, ["a", "b"])
333-
334- ## Single channel
335- single_channel_config_path = os.path.join(self.temp_directory,
336- "single_channel_config")
337+ self.assertEqual(conf.channels['a'].fullcount, 10)
338+ self.assertEqual(conf.channels['a'].versionbase, 1)
339+ self.assertEqual(conf.channels['a'].deltabase, ['a'])
340+
341+ self.assertEqual(conf.channels['b'].fullcount, 0)
342+ self.assertEqual(conf.channels['b'].versionbase, 5)
343+ self.assertEqual(conf.channels['b'].deltabase, ["a", "b"])
344+
345+ def test_missing_mirror(self):
346+ """
347+ If a mirror listed in the global mirrors list doesn't exist, we should
348+ get an error.
349+ """
350+ missing_mirror_config_path = os.path.join(self.temp_directory,
351+ "missing_mirror_config")
352+ with open(missing_mirror_config_path, "w+") as fd:
353+ fd.write("""[global]
354+mirrors = a
355+
356+[mirror_default]
357+ssh_user = user
358+ssh_port = 22
359+ssh_command = command
360+ssh_key = key
361+""")
362+ with self.assertRaises(KeyError) as cm:
363+ config.Config(missing_mirror_config_path)
364+ self.assertEqual(
365+ "'Missing mirror section: mirror_a'", str(cm.exception))
366+
367+ def test_single_channel_configuration(self):
368+ """
369+ A configuration handles a single channel.
370+ """
371+ single_channel_config_path = os.path.join(
372+ self.temp_directory, "single_channel_config")
373 with open(single_channel_config_path, "w+") as fd:
374 fd.write("""[global]
375 channels = a
376@@ -237,24 +303,33 @@
377 files = a
378 file_a = test;arg1;arg2
379 """)
380-
381 conf = config.Config(single_channel_config_path)
382- self.assertEquals(
383+ self.assertEqual(
384 conf.channels['a'].files,
385 [{'name': 'a', 'generator': 'test',
386 'arguments': ['arg1', 'arg2']}])
387
388- ## Invalid channel
389+ def test_invalid_channel_in_configuration(self):
390+ """
391+ We should get an appropriate error message when a named channel is
392+ missing.
393+ """
394 invalid_channel_config_path = os.path.join(self.temp_directory,
395 "invalid_channel_config")
396 with open(invalid_channel_config_path, "w+") as fd:
397 fd.write("""[global]
398 channels = a
399 """)
400-
401- self.assertRaises(KeyError, config.Config, invalid_channel_config_path)
402-
403- ## Invalid file
404+ with self.assertRaises(KeyError) as cm:
405+ config.Config(invalid_channel_config_path)
406+ self.assertEqual(
407+ "'Missing channel section: channel_a'", str(cm.exception))
408+
409+ def test_invalid_files_in_channel(self):
410+ """
411+ We should get an appropriate error message when a channel is
412+ incomplete.
413+ """
414 invalid_file_channel_config_path = os.path.join(
415 self.temp_directory, "invalid_file_channel_config")
416 with open(invalid_file_channel_config_path, "w+") as fd:
417@@ -264,6 +339,38 @@
418 [channel_a]
419 files = a
420 """)
421-
422- self.assertRaises(KeyError, config.Config,
423- invalid_file_channel_config_path)
424+ with self.assertRaises(KeyError) as cm:
425+ config.Config(invalid_file_channel_config_path)
426+ self.assertEqual("'Missing file entry: file_a'", str(cm.exception))
427+
428+ @mock.patch("subprocess.call")
429+ def test_config(self, mock_call):
430+ # Good complete config
431+ config_path = os.path.join(self.temp_directory, "config")
432+ key_path = os.path.join(self.temp_directory, "key")
433+ with open(config_path, "w+") as fd:
434+ fd.write("""[global]
435+base_path = %s
436+mirrors = a, b
437+
438+[mirror_default]
439+ssh_user = user
440+ssh_key = key
441+ssh_port = 22
442+ssh_command = command
443+
444+[mirror_a]
445+ssh_host = hosta
446+
447+[mirror_b]
448+ssh_host = hostb
449+""" % self.temp_directory)
450+
451+ conf = config.Config(config_path)
452+ # Test ssh sync
453+ tools.sync_mirrors(conf)
454+ expected_calls = [((['ssh', '-i', key_path, '-l', 'user',
455+ '-p', '22', 'hosta', 'command'],), {}),
456+ ((['ssh', '-i', key_path, '-l', 'user',
457+ '-p', '22', 'hostb', 'command'],), {})]
458+ self.assertEqual(mock_call.call_args_list, expected_calls)
459
460=== modified file 'tests/test_static.py'
461--- tests/test_static.py 2013-09-10 21:30:30 +0000
462+++ tests/test_static.py 2013-10-01 16:35:07 +0000
463@@ -21,6 +21,8 @@
464 import subprocess
465 import unittest
466
467+from systemimage.tools import find_on_path
468+
469
470 class StaticTests(unittest.TestCase):
471 def all_paths(self):
472@@ -44,7 +46,7 @@
473 paths.append(os.path.join(dirpath, filename))
474 return paths
475
476- @unittest.skipIf(not os.path.exists("/usr/bin/pep8"),
477+ @unittest.skipIf(not find_on_path("pep8"),
478 "Missing pep8, skipping test.")
479 def test_pep8_clean(self):
480 subp = subprocess.Popen(
481@@ -53,9 +55,9 @@
482 output = subp.communicate()[0].splitlines()
483 for line in output:
484 print(line)
485- self.assertEqual(0, len(output))
486+ self.assertEqual(0, len(output), output)
487
488- @unittest.skipIf(not os.path.exists("/usr/bin/pyflakes"),
489+ @unittest.skipIf(not find_on_path("pyflakes"),
490 "Missing pyflakes, skipping test.")
491 def test_pyflakes_clean(self):
492 subp = subprocess.Popen(
493@@ -64,10 +66,10 @@
494 output = subp.communicate()[0].splitlines()
495 for line in output:
496 print(line)
497- self.assertEqual(0, len(output))
498+ self.assertEqual(0, len(output), output)
499
500- @unittest.skipIf(not os.path.exists("/usr/bin/pyflakes3"),
501- "Missing pyflakes, skipping test.")
502+ @unittest.skipIf(not find_on_path("pyflakes3"),
503+ "Missing pyflakes3, skipping test.")
504 def test_pyflakes3_clean(self):
505 subp = subprocess.Popen(
506 ["pyflakes3"] + self.all_paths(),
507
508=== modified file 'utils/check-latest'
509--- utils/check-latest 2013-09-16 22:06:24 +0000
510+++ utils/check-latest 2013-10-01 16:35:07 +0000
511@@ -1,4 +1,4 @@
512-#!/usr/bin/python3
513+#!/usr/bin/env python3
514 # -*- coding: utf-8 -*-
515
516 # Copyright (C) 2013 Canonical Ltd.

Subscribers

People subscribed via source and target branches