Merge ~jugmac00/launchpad:revisionsstatusreport-attach-and-setlog-now-accept-file-object into launchpad:master

[Jürgen Gmach]

RevisionStatusReport.attach/.setLog now also accept a file object

[Colin Watson (cjwatson)]

I'd consider changing `CIUpload` to use this in the same MP. Probably not a major issue in this case, but when doing refactorings like this in the past I've sometimes found that changing the calling code along with the underlying refactoring causes me to notice problems I'd otherwise have missed.

review: Approve

diff --git a/lib/lp/code/interfaces/revisionstatus.py b/lib/lp/code/interfaces/revisionstatus.py
index 90d4e02..8dcb2e4 100644
--- a/lib/lp/code/interfaces/revisionstatus.py
+++ b/lib/lp/code/interfaces/revisionstatus.py
@@ -149,7 +149,8 @@ class IRevisionStatusReportEdit(Interface):
     def setLog(log_data):
         """Set a new log on an existing status report.
 
-        :param log_data: The contents (in bytes) of the log.
+        :param log_data: The contents of the log, either as bytes or as a file
+            object.
         """
 
     # XXX cjwatson 2022-01-14: artifact_type isn't currently exported, but
@@ -166,7 +167,8 @@ class IRevisionStatusReportEdit(Interface):
     def attach(name, data, artifact_type=RevisionStatusArtifactType.BINARY):
         """Attach a new artifact to an existing status report.
 
-        :param data: The contents (in bytes) of the artifact.
+        :param data: The contents of the artifact, either as bytes or as a file
+            object.
         :param artifact_type: The type of the artifact.  This may currently
             only be `RevisionStatusArtifactType.BINARY`, but more types may
             be added in future.
diff --git a/lib/lp/code/model/revisionstatus.py b/lib/lp/code/model/revisionstatus.py
index 8ef5cba..1f74af3 100644
--- a/lib/lp/code/model/revisionstatus.py
+++ b/lib/lp/code/model/revisionstatus.py
@@ -8,6 +8,7 @@ __all__ = [
     ]
 
 import io
+import os
 
 import pytz
 from storm.locals import (
@@ -90,9 +91,18 @@ class RevisionStatusReport(StormBase):
     def setLog(self, log_data):
         filename = '%s-%s.txt' % (self.title, self.commit_sha1)
 
+        if isinstance(log_data, bytes):
+            file = io.BytesIO(log_data)
+            size = len(log_data)
+        else:
+            file = log_data
+            file.seek(0, os.SEEK_END)
+            size = file.tell()
+            file.seek(0)
+
         lfa = getUtility(ILibraryFileAliasSet).create(
-            name=filename, size=len(log_data),
-            file=io.BytesIO(log_data), contentType='text/plain',
+            name=filename, size=size,
+            file=file, contentType='text/plain',
             restricted=self.git_repository.private)
 
         getUtility(IRevisionStatusArtifactSet).new(
@@ -101,8 +111,18 @@ class RevisionStatusReport(StormBase):
     def attach(self, name, data,
                artifact_type=RevisionStatusArtifactType.BINARY):
         """See `IRevisionStatusReport`."""
+
+        if isinstance(data, bytes):
+            file = io.BytesIO(data)
+            size = len(data)
+        else:
+            file = data
+            file.seek(0, os.SEEK_END)
+            size = file.tell()
+            file.seek(0)
+
         lfa = getUtility(ILibraryFileAliasSet).create(
-            name=name, size=len(data), file=io.BytesIO(data),
+            name=name, size=size, file=file,
             contentType='application/octet-stream',
             restricted=self.git_repository.private)
         getUtility(IRevisionStatusArtifactSet).new(lfa, self, artifact_type)
diff --git a/lib/lp/code/model/tests/test_revisionstatus.py b/lib/lp/code/model/tests/test_revisionstatus.py
index 7b5ed39..33f9c3c 100644
--- a/lib/lp/code/model/tests/test_revisionstatus.py
+++ b/lib/lp/code/model/tests/test_revisionstatus.py
@@ -4,9 +4,14 @@
 """Tests for revision status reports and artifacts."""
 
 import hashlib
+from hashlib import sha1
 import io
+import os
 
-from fixtures import FakeLogger
+from fixtures import (
+    FakeLogger,
+    TempDir,
+    )
 import requests
 from storm.store import Store
 from testtools.matchers import (
@@ -30,6 +35,7 @@ from lp.code.interfaces.revisionstatus import (
     IRevisionStatusReportSet,
     )
 from lp.services.auth.enums import AccessTokenScope
+from lp.services.osutils import write_file
 from lp.services.webapp.authorization import check_permission
 from lp.testing import (
     anonymous_logged_in,
@@ -37,6 +43,7 @@ from lp.testing import (
     person_logged_in,
     TestCaseWithFactory,
     )
+from lp.testing.dbuser import switch_dbuser
 from lp.testing.layers import (
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
@@ -195,6 +202,32 @@ class TestRevisionStatusReportWebservice(TestCaseWithFactory):
     def test_setLog_private(self):
         self._test_setLog(private=True)
 
+    def test_setLog_with_file_object(self):
+        switch_dbuser("launchpad_main")
+
+        # create log file
+        path = os.path.join(
+            self.useFixture(TempDir()).path, "test", "build:0.log"
+        )
+        content = "some log content"
+        write_file(path, content.encode("utf-8"))
+
+        report = self.factory.makeRevisionStatusReport(
+            title="build:0",
+            ci_build=self.factory.makeCIBuild(),
+        )
+
+        with person_logged_in(report.creator):
+            with open(path, "rb") as f:
+                report.setLog(f)
+
+        artifacts = list(getUtility(
+            IRevisionStatusArtifactSet).findByReport(report))
+        self.assertEqual(
+            artifacts[0].library_file.content.sha1,
+            sha1(content.encode()).hexdigest()
+        )
+
     def _test_attach(self, private):
         requester = self.factory.makePerson()
         with person_logged_in(requester):
@@ -234,6 +267,32 @@ class TestRevisionStatusReportWebservice(TestCaseWithFactory):
     def test_attach_private(self):
         self._test_attach(private=True)
 
+    def test_attach_with_file_object(self):
+        switch_dbuser("launchpad_main")
+
+        # create text file
+        path = os.path.join(
+            self.useFixture(TempDir()).path, "test.md"
+        )
+        content = "some content"
+        write_file(path, content.encode("utf-8"))
+
+        report = self.factory.makeRevisionStatusReport(
+            title="build:0",
+            ci_build=self.factory.makeCIBuild(),
+        )
+
+        with person_logged_in(report.creator):
+            with open(path, "rb") as f:
+                report.attach("text", f)
+
+        artifacts = list(getUtility(
+            IRevisionStatusArtifactSet).findByReport(report))
+        self.assertEqual(
+            artifacts[0].library_file.content.sha1,
+            sha1(content.encode()).hexdigest()
+        )
+
     def test_update(self):
         report = self.factory.makeRevisionStatusReport(
             result=RevisionStatusResult.FAILED)