Merge lp:~fginther/ubuntu-ci-services-itself/lander-binary-fixes into lp:ubuntu-ci-services-itself

[Francis Ginther]

Add support for processing the removed_binaries list and fix issue of a null added_binaries list.

Add support for processing the removed_binaries list and fix issue of a null added_binaries list.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:290
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/266/
Executed test runs:

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

review: Needs Fixing (continuous-integration)

[Andy Doan (doanac)]

46 + packages = request_parameters.get('added_binaries', '')
47 + if packages:
48 + packages = packages.split(',')

I don't think you need 47 & 48, python's split should be safe there.

63 + parameters = {'added_binaries': ','.join(packages),
64 + 'removed_binaries': None}

Bikeshedding, but most of our code is declaring dictionaries styled like:

  parameters = {
      'added_binaries': ','.join(packages),
      'removed_binaries': None
  }

20 + if added_list:
21 + for package_name in added_list.split(','):
22 + if package_name not in package_list:
23 + # Only add packages not already in the global list
24 + package_list.append(package_name)
25 +
26 + # Finally remove the list of removed binaries from the ticket request
27 + removed_list = request_parameters.get('removed_binaries', '')
28 + if removed_list:
29 + for package_name in removed_list.split(','):
30 + try:
31 + package_list.remove(package_name)
32 + except ValueError:
33 + # This is ok, multiple queued tickets could be trying
34 + # to remove the same binary.
35 + logger.info('Package to remove not found: '
36 + '{}'.format(package_list))
37 +

Not a big deal, and I could see some people advocate what you have there. But I often find it easier to use Python "sets" for things like this. I think you could simplify this to something like:

 packages = set(ticket.get_binaries())
 packages = packages + set(request_parameters.get('added_binaries', '').split(','))
 packages = packages - set(request_parameters.get('removed_binaries', '').split(','))

None of my comments are really a big deal as the code looks like it works to me. I'll just ack the MP and you can decide whether or not you want to do anything with my comments.

review: Approve

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Francis Ginther (fginther)]

> 46 + packages = request_parameters.get('added_binaries', '')
> 47 + if packages:
> 48 + packages = packages.split(',')
>
> I don't think you need 47 & 48, python's split should be safe there.

added_binaries and removed_binaries can be defined as None in which case "AttributeError: 'NoneType' object has no attribute 'split'" can be raised.

> 63 + parameters = {'added_binaries': ','.join(packages),
> 64 + 'removed_binaries': None}
>
> Bikeshedding, but most of our code is declaring dictionaries styled like:
>
> parameters = {
> 'added_binaries': ','.join(packages),
> 'removed_binaries': None
> }

I wasn't aware of this, I can switch.

> 20 + if added_list:
> 21 + for package_name in added_list.split(','):
> 22 + if package_name not in package_list:
> 23 + # Only add packages not already in the global list
> 24 + package_list.append(package_name)
> 25 +
> 26 + # Finally remove the list of removed binaries from the ticket
> request
> 27 + removed_list = request_parameters.get('removed_binaries', '')
> 28 + if removed_list:
> 29 + for package_name in removed_list.split(','):
> 30 + try:
> 31 + package_list.remove(package_name)
> 32 + except ValueError:
> 33 + # This is ok, multiple queued tickets could be trying
> 34 + # to remove the same binary.
> 35 + logger.info('Package to remove not found: '
> 36 + '{}'.format(package_list))
> 37 +
>
> Not a big deal, and I could see some people advocate what you have there. But
> I often find it easier to use Python "sets" for things like this. I think you
> could simplify this to something like:
>
> packages = set(ticket.get_binaries())
> packages = packages + set(request_parameters.get('added_binaries',
> '').split(','))
> packages = packages - set(request_parameters.get('removed_binaries',
> '').split(','))

I like your recommendation here, it's much cleaner. Unit tests are in place, so it's an easy change to test.

> None of my comments are really a big deal as the code looks like it works to
> me. I'll just ack the MP and you can decide whether or not you want to do
> anything with my comments.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[lp:~fginther/ubuntu-ci-services-itself/lander-binary-fixes]

291. By Francis Ginther on 2014-03-03

Use sets for computing the binary package lists, test cleanup.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Francis Ginther (fginther)]

Andy, these changes should address your comments.

review: Needs Resubmitting

[Andy Doan (doanac)]

review: Approve

[Chris Johnston (cjohnston)]

 merge approved

review: Approve

=== modified file 'lander/bin/lander_service_wrapper.py'
--- lander/bin/lander_service_wrapper.py	2014-02-28 09:25:47 +0000
+++ lander/bin/lander_service_wrapper.py	2014-03-03 20:15:56 +0000
@@ -179,17 +179,36 @@
 
 
 def _get_binary_packages(ticket, request_parameters):
-    package_list = []
+    # Start with the set of golden binary packages
+    package_list = set(ticket.get_binaries())
 
     # Extract the list of added packages from the ticket request
-    added_list = request_parameters.get('added_binaries', '')
-    if len(added_list) > 0:
-        package_list = added_list.split(',')
-
-    # Now add the set of golden binary packages
-    package_list.extend(ticket.get_binaries())
-    logger.info('Binaries all: {}'.format(package_list))
-    return package_list
+    try:
+        package_list = package_list | set(
+            request_parameters.get('added_binaries', None).split(','))
+    except AttributeError:
+        # Expected when added_binaries is None.
+        pass
+
+    # Finally remove the list of removed binaries from the ticket request
+    try:
+        removed_list = set(
+            request_parameters.get('removed_binaries', None).split(','))
+        # Log a warning if any package in the removed_binaries is not in the
+        # package_list. This is not an error because multiple queued tickets
+        # may attempt to remove the same package. However, This could be an
+        # indicator of a mis-typed package name.
+        diff_list = removed_list - package_list
+        if diff_list:
+            logger.warning('Package(s) to remove not found: '
+                           '{}'.format(list(diff_list)))
+        package_list = package_list - removed_list
+
+    except AttributeError:
+        # Expected when removed_binaries is None.
+        pass
+
+    return list(package_list)
 
 
 def _handle_image_builder(args):
@@ -232,7 +251,11 @@
     url = '%s/api/v1/ppa_copy/' % config['master']['ppa_assigner_url']
 
     request_parameters = config['master']['request_parameters']
-    packages = request_parameters.get('added_binaries', '').split(',')
+    try:
+        packages = request_parameters.get('added_binaries', None).split(',')
+    except AttributeError:
+        # Expected when added_binaries is None.
+        packages = []
 
     params = {
         'series': request_parameters['series'],

=== modified file 'lander/lander/tests/test_service_wrapper.py'
--- lander/lander/tests/test_service_wrapper.py	2014-02-28 09:25:47 +0000
+++ lander/lander/tests/test_service_wrapper.py	2014-03-03 20:15:56 +0000
@@ -84,44 +84,73 @@
     @mock.patch('lander_service_wrapper.TicketApi')
     def testGetBinaryPackagesAddedOnly(self, api):
         '''Verify that packages from the current ticket are added.'''
-        packages = ['add_package1', 'add_package2']
-        parameters = {'added_binaries': ','.join(packages)}
+        packages = ['add_package1']
+        parameters = {
+            'added_binaries': ','.join(packages),
+            'removed_binaries': None}
         api.get_binaries = mock.Mock(return_value=[])
         actual = lander_service_wrapper._get_binary_packages(api, parameters)
-        self.assertEqual(packages, actual)
+        self.assertItemsEqual(packages, actual)
 
     @mock.patch('lander_service_wrapper.TicketApi')
     def testGetBinaryPackagesNone(self, api):
         '''Verify that no packages are added when none are specified.'''
-        packages = []
-        parameters = {'added_binaries': ','.join(packages)}
-        api.get_binaries = mock.Mock(return_value=[])
-        actual = lander_service_wrapper._get_binary_packages(api, parameters)
-        self.assertEqual(packages, actual)
+        parameters = {
+            'added_binaries': None,
+            'removed_binaries': None}
+        api.get_binaries = mock.Mock(return_value=[])
+        actual = lander_service_wrapper._get_binary_packages(api, parameters)
+        self.assertEqual([], actual)
+
+    @mock.patch('lander_service_wrapper.TicketApi')
+    def testGetBinaryPackagesRemovedOnly(self, api):
+        '''Verify that removing packages that aren't there is ok.'''
+        packages = ['remove_package1', 'remove_package2']
+        parameters = {
+            'added_binaries': None,
+            'removed_binaries': ','.join(packages)}
+        api.get_binaries = mock.Mock(return_value=[])
+        actual = lander_service_wrapper._get_binary_packages(api, parameters)
+        self.assertEqual([], actual)
 
     @mock.patch('lander_service_wrapper.TicketApi')
     def testGetBinaryPackagesGlobalOnly(self, api):
         '''Verify that packages only from the global list are added.'''
-        packages = []
         golden = ['golden_package1', 'golden_package2']
-        parameters = {'added_binaries': ','.join(packages)}
+        parameters = {
+            'added_binaries': None,
+            'removed_binaries': None}
         api.get_binaries = mock.Mock(return_value=golden)
         actual = lander_service_wrapper._get_binary_packages(api, parameters)
-        expected = packages
-        expected.extend(golden)
-        self.assertEqual(packages, actual)
+        self.assertItemsEqual(golden, actual)
 
     @mock.patch('lander_service_wrapper.TicketApi')
     def testGetBinaryPackagesAddedAndGlobal(self, api):
         '''Verify that packages from the ticket and global list are added.'''
         packages = ['add_package1', 'add_package2']
         golden = ['golden_package1', 'golden_package2']
-        parameters = {'added_binaries': ','.join(packages)}
-        api.get_binaries = mock.Mock(return_value=golden)
-        actual = lander_service_wrapper._get_binary_packages(api, parameters)
-        expected = packages
-        expected.extend(golden)
-        self.assertEqual(packages, actual)
+        parameters = {
+            'added_binaries': ','.join(packages),
+            'removed_binaries': None}
+        api.get_binaries = mock.Mock(return_value=golden)
+        actual = lander_service_wrapper._get_binary_packages(api, parameters)
+        expected = golden
+        expected.extend(packages)
+        self.assertItemsEqual(expected, actual)
+
+    @mock.patch('lander_service_wrapper.TicketApi')
+    def testGetBinaryPackagesAddedRemovedAndGlobal(self, api):
+        '''Verify that packages from all three lists are processed.'''
+        add_packages = ['add_package1', 'add_package2']
+        remove_packages = ['add_package1', 'golden_package2']
+        golden = ['golden_package1', 'golden_package2']
+        parameters = {
+            'added_binaries': ','.join(add_packages),
+            'removed_binaries': ','.join(remove_packages)}
+        api.get_binaries = mock.Mock(return_value=golden)
+        actual = lander_service_wrapper._get_binary_packages(api, parameters)
+        expected = ['golden_package1', 'add_package2']
+        self.assertItemsEqual(expected, actual)
 
     @mock.patch('lander_service_wrapper._post')
     def testPPAAssigner404(self, post):