launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:code-import-scheduler-librarian into launchpad:master.

Commit message:
Accept code import log file data over XML-RPC

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/391793

The code import worker currently uploads its log file directly to the librarian.  This is a bit of an obstacle to splitting out the code import worker from the main Launchpad codebase: it wouldn't necessarily be impossible to retain the remote librarian upload client in a future lp-codeimport, but it would be somewhat awkward as that client is currently designed with the expectation of having direct Launchpad database access.

Log files aren't typically all that large, though, so it seems workable to just send them as part of the XML-RPC `finishJobID` request and have the webapp side of that request do the librarian upload instead.  This prepares for that by accepting either XML-RPC-encapsulated binary data or a librarian URL as the third parameter to `finishJobID`.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:code-import-scheduler-librarian into launchpad:master.

diff --git a/lib/lp/code/interfaces/codeimportscheduler.py b/lib/lp/code/interfaces/codeimportscheduler.py
index c15873e..e0d646e 100644
--- a/lib/lp/code/interfaces/codeimportscheduler.py
+++ b/lib/lp/code/interfaces/codeimportscheduler.py
@@ -54,8 +54,14 @@ class ICodeImportScheduler(Interface):
         :raise NoSuchCodeImportJob: if no job with id `job_id` exists.
         """
 
-    def finishJobID(job_id, status_name, log_file_alias_url):
+    def finishJobID(job_id, status_name, log_file):
         """Call `ICodeImportJobWorkflow.finishJob` for job `job_id`.
 
+        :param job_id: The ID of the code import job to finish.
+        :param status_name: The outcome of the job as the name of a
+            `CodeImportResultStatus` item.
+        :param log_file: A log file to display for diagnostics, either as a
+            `six.moves.xmlrpc_client.Binary` containing the log file data or
+            as the URL of a file in the librarian.
         :raise NoSuchCodeImportJob: if no job with id `job_id` exists.
         """
diff --git a/lib/lp/code/xmlrpc/codeimportscheduler.py b/lib/lp/code/xmlrpc/codeimportscheduler.py
index 35ba346..be3e5e2 100644
--- a/lib/lp/code/xmlrpc/codeimportscheduler.py
+++ b/lib/lp/code/xmlrpc/codeimportscheduler.py
@@ -8,7 +8,10 @@ __all__ = [
     'CodeImportSchedulerAPI',
     ]
 
+import io
+
 import six
+from six.moves import xmlrpc_client
 from zope.component import getUtility
 from zope.interface import implementer
 from zope.security.proxy import removeSecurityProxy
@@ -61,11 +64,10 @@ class CodeImportSchedulerAPI(LaunchpadXMLRPCView):
         """See `ICodeImportScheduler`."""
         return self._updateHeartbeat(job_id, six.ensure_text(log_tail))
 
-    def finishJobID(self, job_id, status_name, log_file_alias_url):
+    def finishJobID(self, job_id, status_name, log_file):
         """See `ICodeImportScheduler`."""
         return self._finishJobID(
-            job_id, six.ensure_text(status_name),
-            six.ensure_text(log_file_alias_url))
+            job_id, six.ensure_text(status_name), log_file)
 
     @return_fault
     def _getImportDataForJobID(self, job_id):
@@ -87,14 +89,24 @@ class CodeImportSchedulerAPI(LaunchpadXMLRPCView):
         return 0
 
     @return_fault
-    def _finishJobID(self, job_id, status_name, log_file_alias_url):
+    def _finishJobID(self, job_id, status_name, log_file):
         job = self._getJob(job_id)
         status = CodeImportResultStatus.items[status_name]
         workflow = removeSecurityProxy(getUtility(ICodeImportJobWorkflow))
-        if log_file_alias_url:
+        if isinstance(log_file, xmlrpc_client.Binary):
+            log_file_name = '%s.log' % (
+                job.code_import.target.unique_name[1:].replace('/', '-'))
+            log_file_alias = getUtility(ILibraryFileAliasSet).create(
+                log_file_name, len(log_file.data), io.BytesIO(log_file.data),
+                'text/plain')
+        elif log_file:
+            # XXX cjwatson 2020-10-05: Backward compatibility for previous
+            # versions that uploaded the log file to the librarian from the
+            # scheduler; remove this once deployed code import machines no
+            # longer need this.
             library_file_alias_set = getUtility(ILibraryFileAliasSet)
             # XXX This is so so so terrible:
-            log_file_alias_id = int(log_file_alias_url.split('/')[-2])
+            log_file_alias_id = int(six.ensure_text(log_file).split('/')[-2])
             log_file_alias = library_file_alias_set[log_file_alias_id]
         else:
             log_file_alias = None
diff --git a/lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py b/lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py
index c79f741..9a87646 100644
--- a/lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py
+++ b/lib/lp/code/xmlrpc/tests/test_codeimportscheduler.py
@@ -6,6 +6,7 @@
 __metaclass__ = type
 
 from six.moves import xmlrpc_client
+import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
@@ -113,7 +114,7 @@ class TestCodeImportSchedulerAPI(TestCaseWithFactory):
         self.assertSqlAttributeEqualsDate(
             code_import, 'date_last_successful', UTC_NOW)
 
-    def test_finishJobID_with_log_file(self):
+    def test_finishJobID_with_log_file_alias_url(self):
         # finishJobID calls the finishJobID job workflow method and can parse
         # a librarian file's http url to figure out its ID.
         code_import_job = self.makeCodeImportJob(running=True)
@@ -125,6 +126,18 @@ class TestCodeImportSchedulerAPI(TestCaseWithFactory):
         self.assertEqual(
             log_file_alias, code_import.results.last().log_file)
 
+    def test_finishJobID_with_log_file_data(self):
+        # finishJobID calls the finishJobID job workflow method and uploads
+        # log file data to the librarian.
+        code_import_job = self.makeCodeImportJob(running=True)
+        code_import = code_import_job.code_import
+        self.api.finishJobID(
+            code_import_job.id, CodeImportResultStatus.SUCCESS.name,
+            xmlrpc_client.Binary(b'log file data\n'))
+        transaction.commit()
+        self.assertEqual(
+            b'log file data\n', code_import.results.last().log_file.read())
+
     def test_finishJobID_not_found(self):
         # getImportDataForJobID returns a NoSuchCodeImportJob fault when there
         # is no code import job with the given ID.