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

[Chris Johnston]

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

[Andy Doan (doanac)]

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.

[PS Jenkins bot (ps-jenkins)]

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]

298. By Chris Johnston on 2014-03-03

Rename function to assert_valid_package_list, return nothing or raise error

[PS Jenkins bot (ps-jenkins)]

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]

299. By Chris Johnston on 2014-03-03

Add a new unit test for checking an invalid entry

[PS Jenkins bot (ps-jenkins)]

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)

[Andy Doan (doanac)]

review: Approve

[Chris Johnston (cjohnston)]

 merge approved

review: Approve

=== 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 @@
 AUTH_CONFIG = None
 
 
+class InputError(Exception):
+    """Exception raised for errors in the input."""
+
+
 def load_config(config_file):
     global CI_URL
     global AUTH_CONFIG
@@ -102,3 +106,9 @@
         # Package not found.
         log.info("Source package not found: %s" % sourcepackage)
         return ''
+
+
+def assert_valid_package_list(package_list_str):
+    if package_list_str and " " in package_list_str:
+        raise InputError("Added and removed packages have to be "
+                         "comma-separated lists with no spaces.")

=== 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 @@
                                'data/foobar_0.1-1_source.changes')
         args = ['create_ticket', '-t', 'New feature', '-b', '1234', '-o',
                 '[e-mail]', '-d', 'This is a cool new feature',
-                '-a', 'foo', '-r', 'baz', '-s', changes]
+                '-a', 'foo', '-r', 'bar,baz', '-s', changes]
 
         args = self.cli.parse_arguments(args)
         new_ticket_ = new_ticket(args)
@@ -177,7 +177,7 @@
 
         args = ['create_ticket', '-t', '"New feature"', '-b', '4321', '-o',
                 '[e-mail]', '-d', '"This is a cool new feature"',
-                '-a', 'foo', '-r', 'baz', '-s', changes_1, '-s', changes_2]
+                '-a', 'foo,bar', '-r', 'baz', '-s', changes_1, '-s', changes_2]
 
         args = self.cli.parse_arguments(args)
         new_ticket_ = new_ticket(args)
@@ -295,3 +295,46 @@
             lc.records[0].msg,
             "Problem validating .changes file: Invalid 'Distribution' in "
             ".changes: UNRELEASED")
+
+    @mock.patch('ci_libs.ticket.SubTicket._process')
+    @mock.patch('ci_libs.utils.post',
+                return_value='http://www.example.com/api/v1/ticket/39/')
+    def test_is_valid_package_list_add_space(self, mock_patch, mock_post):
+        """
+        Test a ticket adding two test binaries and not removing any binaries
+        fails
+        """
+        changes = os.path.join(os.path.dirname(__file__),
+                               'data/foobar_0.1-1_source.changes')
+        args = ['create_ticket', '-t', '"New feature"', '-d',
+                '"This is a cool new feature"', '-o', '[e-mail]',
+                '-a', 'foo, bar', '-s', changes]
+        with LogCapture() as lc:
+            logger = logging.getLogger()
+            self.cli.main(args, log=logger)
+        self.assertIsNotNone(lc)
+        self.assertEquals(
+            lc.records[0].msg[0],
+            "Added and removed packages have to be comma-separated lists "
+            "with no spaces.")
+
+    @mock.patch('ci_libs.ticket.SubTicket._process')
+    @mock.patch('ci_libs.utils.post',
+                return_value='http://www.example.com/api/v1/ticket/39/')
+    def test_is_valid_package_list_remove_space(self, mock_patch, mock_post):
+        """
+        Test a ticket removing two test binaries fails
+        """
+        changes = os.path.join(os.path.dirname(__file__),
+                               'data/foobar_0.1-1_source.changes')
+        args = ['create_ticket', '-t', '"New feature"', '-d',
+                '"This is a cool new feature"', '-o', '[e-mail]',
+                '-a', 'foo', '-r', 'bar, baz', '-s', changes]
+        with LogCapture() as lc:
+            logger = logging.getLogger()
+            self.cli.main(args, log=logger)
+        self.assertIsNotNone(lc)
+        self.assertEquals(
+            lc.records[0].msg[0],
+            "Added and removed packages have to be comma-separated lists "
+            "with no spaces.")

=== 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 @@
 import json
 import mock
 import tempfile
+import logging
 
 from tests import CLITestCase
 from ci_libs import utils
@@ -68,7 +69,6 @@
 
     def test_load_config(self):
         with tempfile.NamedTemporaryFile() as config_file:
-            import logging
             logging.warn("config_file: %s", config_file)
             logging.warn("config_file.name: %s", config_file.name)
             with open(config_file.name, 'w') as fp:
@@ -77,3 +77,16 @@
             utils.load_config(config_file.name)
 
         self.assertEqual('http://example.com/ci', utils.CI_URL)
+
+    def test_assert_valid_package_list_single(self):
+        utils.assert_valid_package_list('foo')
+
+    def test_assert_valid_package_list_three(self):
+        utils.assert_valid_package_list('foo,bar,baz')
+
+    def test_assert_valid_package_list_empty(self):
+        utils.assert_valid_package_list(package_list_str=None)
+
+    def test_assert_valid_package_list_with_space(self):
+        with self.assertRaises(utils.InputError):
+            utils.assert_valid_package_list('bar, baz')

=== 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 @@
                     raise ChangesFileNotFound(source)
                 if not source.endswith(".changes"):
                     raise NotAChangesFileError(source)
+            utils.assert_valid_package_list(args.add)
+            utils.assert_valid_package_list(args.remove)
 
         except AttributeError:
             # We're not in create_ticket context, moving on.
@@ -111,6 +113,8 @@
         utils.load_config(DEF_CFG)
         args.func(args)
         return 0
+    except utils.InputError as exc:
+        log.error(exc)
     except ChangesFileNotFound as exc:
         log.error("Changes file not found: {}".format(exc))
     except NotAChangesFileError as exc: