launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/code-import-data-accept-dict into lp:launchpad.

Commit message:
Make the code import worker monitor accept a dict as job data from the scheduler.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/code-import-data-accept-dict/+merge/341485

This is easier to extend than a list: in particular, it's much easier to add optional arguments.  I have a couple of use cases in mind for this (passing macaroons in the environment rather than on the command line, and fetching blacklisted hostnames from the scheduler rather than directly from lp.services.config to improve decoupling), but in general I think lists are a poor choice for this kind of thing anyway.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/code-import-data-accept-dict into lp:launchpad.

=== modified file 'lib/lp/code/interfaces/codeimportscheduler.py'
--- lib/lp/code/interfaces/codeimportscheduler.py	2013-01-07 02:40:55 +0000
+++ lib/lp/code/interfaces/codeimportscheduler.py	2018-03-15 22:14:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Code import scheduler interfaces."""
@@ -38,13 +38,13 @@
     def getImportDataForJobID(job_id):
         """Get data about the import with job id `job_id`.
 
-        :return: ``(worker_arguments, branch_url, log_file_name)`` where:
+        :return: ``(worker_arguments, target_url, log_file_name)`` where:
             * ``worker_arguments`` are the arguments to pass to the code
-              import worker subprocess.
-           * ``branch_url`` is the URL of the import branch (only used to put
-             in OOPS reports)
-           * ``log_file_name`` is the name of the log file to create in the
-             librarian.
+              import worker subprocess
+            * ``target_url`` is the URL of the import branch/repository
+              (only used in OOPS reports)
+            * ``log_file_name`` is the name of the log file to create in the
+              librarian.
         :raise NoSuchCodeImportJob: if no job with id `job_id` exists.
         """
 

=== modified file 'lib/lp/codehosting/codeimport/tests/test_workermonitor.py'
--- lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2017-12-19 17:22:44 +0000
+++ lib/lp/codehosting/codeimport/tests/test_workermonitor.py	2018-03-15 22:14:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for the CodeImportWorkerMonitor and related classes."""
@@ -247,6 +247,17 @@
         return worker_monitor.getWorkerArguments().addCallback(
             self.assertEqual, args)
 
+    def test_getWorkerArguments_dict(self):
+        # getWorkerArguments returns a deferred that fires with the
+        # 'arguments' part of what getImportDataForJobID returns.
+        # (New protocol: data passed as a dict.)
+        args = [self.factory.getUniqueString(),
+                self.factory.getUniqueString()]
+        data = {'arguments': args, 'target_url': 1, 'log_file_name': 2}
+        worker_monitor = self.makeWorkerMonitorWithJob(1, data)
+        return worker_monitor.getWorkerArguments().addCallback(
+            self.assertEqual, args)
+
     def test_getWorkerArguments_sets_target_url_and_logfilename(self):
         # getWorkerArguments sets the _target_url (for use in oops reports)
         # and _log_file_name (for upload to the librarian) attributes on the
@@ -266,6 +277,30 @@
         return worker_monitor.getWorkerArguments().addCallback(
             check_branch_log)
 
+    def test_getWorkerArguments_sets_target_url_and_logfilename_dict(self):
+        # getWorkerArguments sets the _target_url (for use in oops reports)
+        # and _log_file_name (for upload to the librarian) attributes on the
+        # WorkerMonitor from the data returned by getImportDataForJobID.
+        # (New protocol: data passed as a dict.)
+        target_url = self.factory.getUniqueString()
+        log_file_name = self.factory.getUniqueString()
+        data = {
+            'arguments': ['a'],
+            'target_url': target_url,
+            'log_file_name': log_file_name,
+            }
+        worker_monitor = self.makeWorkerMonitorWithJob(1, data)
+
+        def check_branch_log(ignored):
+            # Looking at the _ attributes here is in slightly poor taste, but
+            # much much easier than them by logging and parsing an oops, etc.
+            self.assertEqual(
+                (target_url, log_file_name),
+                (worker_monitor._target_url, worker_monitor._log_file_name))
+
+        return worker_monitor.getWorkerArguments().addCallback(
+            check_branch_log)
+
     def test_getWorkerArguments_job_not_found_raises_exit_quietly(self):
         # When getImportDataForJobID signals a fault indicating that
         # getWorkerArguments didn't find the supplied job, getWorkerArguments

=== modified file 'lib/lp/codehosting/codeimport/workermonitor.py'
--- lib/lp/codehosting/codeimport/workermonitor.py	2016-10-11 13:41:32 +0000
+++ lib/lp/codehosting/codeimport/workermonitor.py	2018-03-15 22:14:32 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Code to talk to the database about what the worker script is doing."""
@@ -176,7 +176,14 @@
             'getImportDataForJobID', self._job_id)
 
         def _processResult(result):
-            code_import_arguments, target_url, log_file_name = result
+            if isinstance(result, dict):
+                code_import_arguments = result['arguments']
+                target_url = result['target_url']
+                log_file_name = result['log_file_name']
+            else:
+                # XXX cjwatson 2018-03-15: Remove once the scheduler always
+                # sends a dict.
+                code_import_arguments, target_url, log_file_name = result
             self._target_url = target_url
             self._log_file_name = log_file_name
             self._logger.info(