Ubuntu CI Services

Merge lp:~cjohnston/ubuntu-ci-services-itself/reject-bad-lists into lp:ubuntu-ci-services-itself

  1. reject-bad-lists
  2. Merge into trunk
Proposed by Chris Johnston
Status: Merged
Approved by: Chris Johnston
Approved revision: 299
Merged at revision: 302
Proposed branch: lp:~cjohnston/ubuntu-ci-services-itself/reject-bad-lists
Merge into: lp:ubuntu-ci-services-itself
Diff against target: 152 lines (+73/-3)
4 files modified
cli/ci_libs/utils.py (+10/-0)
cli/tests/test_cli.py (+45/-2)
cli/tests/test_utils.py (+14/-1)
cli/ubuntu-ci (+4/-0)
To merge this branch: bzr merge lp:~cjohnston/ubuntu-ci-services-itself/reject-bad-lists
Reviewer Review Type Date Requested Status
Chris Johnston (community) Approve
Andy Doan (community) Approve
PS Jenkins bot (community) continuous-integration Approve
Review via email: mp+208519@code.launchpad.net

Commit message

Make the CLI check the formatting of -a and -r to ensure that there are no spaces

To post a comment you must log in.
Revision history for this message
Andy Doan (doanac) wrote :

don't want to sound like bike-shedding, but this confused me as I read. you have a function name:

21 +def is_valid_package_list(package_list_str):

but it returns true if it passes, and throws an exception if it fails. I think you want something like:

 def assert_valid_package_list(package_list_str)

the function returns nothing, but throws an exception if the list isn't correct.

My last comment is super shedding, so i'd just say ignore me. However, WRT "utils.InputError", I think just using Python's built-in exception "ValueError" might make more sense.

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:297
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/276/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/276/rebuild

review: Approve (continuous-integration)
lp:~cjohnston/ubuntu-ci-services-itself/reject-bad-lists updated
298. By Chris Johnston

Rename function to assert_valid_package_list, return nothing or raise error

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:298
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/279/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/279/rebuild

review: Approve (continuous-integration)
lp:~cjohnston/ubuntu-ci-services-itself/reject-bad-lists updated
299. By Chris Johnston

Add a new unit test for checking an invalid entry

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:299
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/280/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/280/rebuild

review: Approve (continuous-integration)
Revision history for this message
Andy Doan (doanac) :
review: Approve
Revision history for this message
Chris Johnston (cjohnston) wrote :

 merge approved

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'cli/ci_libs/utils.py'
--- cli/ci_libs/utils.py 2014-02-17 10:13:29 +0000
+++ cli/ci_libs/utils.py 2014-03-03 22:45:18 +0000
@@ -36,6 +36,10 @@
36AUTH_CONFIG = None36AUTH_CONFIG = None
3737
3838
39class InputError(Exception):
40 """Exception raised for errors in the input."""
41
42
39def load_config(config_file):43def load_config(config_file):
40 global CI_URL44 global CI_URL
41 global AUTH_CONFIG45 global AUTH_CONFIG
@@ -102,3 +106,9 @@
102 # Package not found.106 # Package not found.
103 log.info("Source package not found: %s" % sourcepackage)107 log.info("Source package not found: %s" % sourcepackage)
104 return ''108 return ''
109
110
111def assert_valid_package_list(package_list_str):
112 if package_list_str and " " in package_list_str:
113 raise InputError("Added and removed packages have to be "
114 "comma-separated lists with no spaces.")
105115
=== modified file 'cli/tests/test_cli.py'
--- cli/tests/test_cli.py 2014-02-28 20:09:45 +0000
+++ cli/tests/test_cli.py 2014-03-03 22:45:18 +0000
@@ -154,7 +154,7 @@
154 'data/foobar_0.1-1_source.changes')154 'data/foobar_0.1-1_source.changes')
155 args = ['create_ticket', '-t', 'New feature', '-b', '1234', '-o',155 args = ['create_ticket', '-t', 'New feature', '-b', '1234', '-o',
156 'someone@example.com', '-d', 'This is a cool new feature',156 'someone@example.com', '-d', 'This is a cool new feature',
157 '-a', 'foo', '-r', 'baz', '-s', changes]157 '-a', 'foo', '-r', 'bar,baz', '-s', changes]
158158
159 args = self.cli.parse_arguments(args)159 args = self.cli.parse_arguments(args)
160 new_ticket_ = new_ticket(args)160 new_ticket_ = new_ticket(args)
@@ -177,7 +177,7 @@
177177
178 args = ['create_ticket', '-t', '"New feature"', '-b', '4321', '-o',178 args = ['create_ticket', '-t', '"New feature"', '-b', '4321', '-o',
179 'someone@example.com', '-d', '"This is a cool new feature"',179 'someone@example.com', '-d', '"This is a cool new feature"',
180 '-a', 'foo', '-r', 'baz', '-s', changes_1, '-s', changes_2]180 '-a', 'foo,bar', '-r', 'baz', '-s', changes_1, '-s', changes_2]
181181
182 args = self.cli.parse_arguments(args)182 args = self.cli.parse_arguments(args)
183 new_ticket_ = new_ticket(args)183 new_ticket_ = new_ticket(args)
@@ -295,3 +295,46 @@
295 lc.records[0].msg,295 lc.records[0].msg,
296 "Problem validating .changes file: Invalid 'Distribution' in "296 "Problem validating .changes file: Invalid 'Distribution' in "
297 ".changes: UNRELEASED")297 ".changes: UNRELEASED")
298
299 @mock.patch('ci_libs.ticket.SubTicket._process')
300 @mock.patch('ci_libs.utils.post',
301 return_value='http://www.example.com/api/v1/ticket/39/')
302 def test_is_valid_package_list_add_space(self, mock_patch, mock_post):
303 """
304 Test a ticket adding two test binaries and not removing any binaries
305 fails
306 """
307 changes = os.path.join(os.path.dirname(__file__),
308 'data/foobar_0.1-1_source.changes')
309 args = ['create_ticket', '-t', '"New feature"', '-d',
310 '"This is a cool new feature"', '-o', 'someone@example.com',
311 '-a', 'foo, bar', '-s', changes]
312 with LogCapture() as lc:
313 logger = logging.getLogger()
314 self.cli.main(args, log=logger)
315 self.assertIsNotNone(lc)
316 self.assertEquals(
317 lc.records[0].msg[0],
318 "Added and removed packages have to be comma-separated lists "
319 "with no spaces.")
320
321 @mock.patch('ci_libs.ticket.SubTicket._process')
322 @mock.patch('ci_libs.utils.post',
323 return_value='http://www.example.com/api/v1/ticket/39/')
324 def test_is_valid_package_list_remove_space(self, mock_patch, mock_post):
325 """
326 Test a ticket removing two test binaries fails
327 """
328 changes = os.path.join(os.path.dirname(__file__),
329 'data/foobar_0.1-1_source.changes')
330 args = ['create_ticket', '-t', '"New feature"', '-d',
331 '"This is a cool new feature"', '-o', 'someone@example.com',
332 '-a', 'foo', '-r', 'bar, baz', '-s', changes]
333 with LogCapture() as lc:
334 logger = logging.getLogger()
335 self.cli.main(args, log=logger)
336 self.assertIsNotNone(lc)
337 self.assertEquals(
338 lc.records[0].msg[0],
339 "Added and removed packages have to be comma-separated lists "
340 "with no spaces.")
298341
=== modified file 'cli/tests/test_utils.py'
--- cli/tests/test_utils.py 2014-02-17 10:13:29 +0000
+++ cli/tests/test_utils.py 2014-03-03 22:45:18 +0000
@@ -18,6 +18,7 @@
18import json18import json
19import mock19import mock
20import tempfile20import tempfile
21import logging
2122
22from tests import CLITestCase23from tests import CLITestCase
23from ci_libs import utils24from ci_libs import utils
@@ -68,7 +69,6 @@
6869
69 def test_load_config(self):70 def test_load_config(self):
70 with tempfile.NamedTemporaryFile() as config_file:71 with tempfile.NamedTemporaryFile() as config_file:
71 import logging
72 logging.warn("config_file: %s", config_file)72 logging.warn("config_file: %s", config_file)
73 logging.warn("config_file.name: %s", config_file.name)73 logging.warn("config_file.name: %s", config_file.name)
74 with open(config_file.name, 'w') as fp:74 with open(config_file.name, 'w') as fp:
@@ -77,3 +77,16 @@
77 utils.load_config(config_file.name)77 utils.load_config(config_file.name)
7878
79 self.assertEqual('http://example.com/ci', utils.CI_URL)79 self.assertEqual('http://example.com/ci', utils.CI_URL)
80
81 def test_assert_valid_package_list_single(self):
82 utils.assert_valid_package_list('foo')
83
84 def test_assert_valid_package_list_three(self):
85 utils.assert_valid_package_list('foo,bar,baz')
86
87 def test_assert_valid_package_list_empty(self):
88 utils.assert_valid_package_list(package_list_str=None)
89
90 def test_assert_valid_package_list_with_space(self):
91 with self.assertRaises(utils.InputError):
92 utils.assert_valid_package_list('bar, baz')
8093
=== modified file 'cli/ubuntu-ci'
--- cli/ubuntu-ci 2014-02-28 20:09:45 +0000
+++ cli/ubuntu-ci 2014-03-03 22:45:18 +0000
@@ -103,6 +103,8 @@
103 raise ChangesFileNotFound(source)103 raise ChangesFileNotFound(source)
104 if not source.endswith(".changes"):104 if not source.endswith(".changes"):
105 raise NotAChangesFileError(source)105 raise NotAChangesFileError(source)
106 utils.assert_valid_package_list(args.add)
107 utils.assert_valid_package_list(args.remove)
106108
107 except AttributeError:109 except AttributeError:
108 # We're not in create_ticket context, moving on.110 # We're not in create_ticket context, moving on.
@@ -111,6 +113,8 @@
111 utils.load_config(DEF_CFG)113 utils.load_config(DEF_CFG)
112 args.func(args)114 args.func(args)
113 return 0115 return 0
116 except utils.InputError as exc:
117 log.error(exc)
114 except ChangesFileNotFound as exc:118 except ChangesFileNotFound as exc:
115 log.error("Changes file not found: {}".format(exc))119 log.error("Changes file not found: {}".format(exc))
116 except NotAChangesFileError as exc:120 except NotAChangesFileError as exc:

Subscribers

People subscribed via source and target branches