launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/queue-api-custom-files into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1006173 in Launchpad itself: "Queue tool requires direct DB access"
  https://bugs.launchpad.net/launchpad/+bug/1006173

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/queue-api-custom-files/+merge/113574

== Summary ==

Fix a slight flaw in https://code.launchpad.net/~cjwatson/launchpad/queue-api-readonly/+merge/113202; there's no sensible way to get details of custom uploads.

== Proposed fix ==

I considered changing customFileUrls to customFiles and adding metadata there, but on reflection that is not ideal because the queue API client would have to add a call to customFiles for every build upload in the queue.  A lighter-weight approach is to add custom file properties to getBinaryProperties (the separation between binaries and custom files is in some ways a bit artificial anyway), so the client just gets back a dict with some different keys.

Since there are no production clients for this yet, this slight interface change should be fine.

== LOC Rationale ==

+43, but I'm still comfortably within the limit for this whole arc of work as described in https://code.launchpad.net/~cjwatson/launchpad/queue-api/+merge/108967.

== Tests ==

bin/test -vvct TestPackageUploadWebservice

== Demo and Q/A ==

As with https://code.launchpad.net/~cjwatson/launchpad/queue-api-readonly/+merge/113202; just make sure it looks sensible for builds with custom uploads attached.
-- 
https://code.launchpad.net/~cjwatson/launchpad/queue-api-custom-files/+merge/113574
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/queue-api-custom-files into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py	2012-07-04 13:02:32 +0000
+++ lib/lp/soyuz/model/queue.py	2012-07-05 13:10:27 +0000
@@ -320,8 +320,14 @@
 
     def getBinaryProperties(self):
         """See `IPackageUpload`."""
-        return list(chain.from_iterable(
+        properties = list(chain.from_iterable(
             build.binaries for build in self.builds))
+        for file in self.customfiles:
+            properties.append({
+                "name": file.libraryfilealias.filename,
+                "customformat": file.customformat.title,
+                })
+        return properties
 
     def setNew(self):
         """See `IPackageUpload`."""

=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py	2012-07-04 13:02:32 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py	2012-07-05 13:10:27 +0000
@@ -1034,6 +1034,18 @@
                 for file in upload.sourcepackagerelease.files]
         self.assertContentEqual(source_file_urls, ws_source_file_urls)
 
+    def assertBinaryPropertiesMatch(self, arch, bpr, ws_binary):
+        expected_binary = {
+            "is_new": True,
+            "name": bpr.name,
+            "version": bpr.version,
+            "architecture": arch,
+            "component": "universe",
+            "section": bpr.section.name,
+            "priority": bpr.priority.name,
+            }
+        self.assertEqual(expected_binary, ws_binary)
+
     def test_binary_info(self):
         # API clients can inspect properties of binary uploads.
         person = self.makeQueueAdmin([self.universe])
@@ -1047,19 +1059,8 @@
         self.assertTrue(ws_upload.contains_build)
         ws_binaries = ws_upload.getBinaryProperties()
         self.assertEqual(len(list(bprs)), len(ws_binaries))
-        for bpr, binary in zip(bprs, ws_binaries):
-            expected_binary = {
-                "is_new": True,
-                "name": bpr.name,
-                "version": bpr.version,
-                "architecture": arch,
-                "component": "universe",
-                "section": bpr.section.name,
-                "priority": bpr.priority.name,
-                }
-            self.assertContentEqual(expected_binary.keys(), binary.keys())
-            for key, value in expected_binary.items():
-                self.assertEqual(value, binary[key])
+        for bpr, ws_binary in zip(bprs, ws_binaries):
+            self.assertBinaryPropertiesMatch(arch, bpr, ws_binary)
 
     def test_binary_fetch(self):
         # API clients can fetch files attached to binary uploads.
@@ -1091,6 +1092,13 @@
             "debian-installer-images_1.tar.gz", ws_upload.display_name)
         self.assertEqual("-", ws_upload.display_version)
         self.assertEqual("raw-installer", ws_upload.display_arches)
+        ws_binaries = ws_upload.getBinaryProperties()
+        self.assertEqual(1, len(ws_binaries))
+        expected_binary = {
+            "name": "debian-installer-images_1.tar.gz",
+            "customformat": "raw-installer",
+            }
+        self.assertEqual(expected_binary, ws_binaries[0])
 
     def test_custom_fetch(self):
         # API clients can fetch files attached to custom uploads.
@@ -1106,3 +1114,32 @@
                     file.libraryfilealias, upload.archive).http_url
                 for file in upload.customfiles]
         self.assertContentEqual(custom_file_urls, ws_custom_file_urls)
+
+    def test_binary_and_custom_info(self):
+        # API clients can inspect properties of uploads containing both
+        # ordinary binaries and custom upload files.
+        person = self.makeQueueAdmin([self.universe])
+        upload, _ = self.makeBinaryPackageUpload(
+            person, binarypackagename="foo", component=self.universe)
+        with person_logged_in(person):
+            arch = upload.builds[0].build.arch_tag
+            bprs = upload.builds[0].build.binarypackages
+            version = bprs[0].version
+            lfa = self.factory.makeLibraryFileAlias(
+                filename="foo_%s_%s_translations.tar.gz" % (version, arch))
+            upload.addCustom(
+                lfa, PackageUploadCustomFormat.ROSETTA_TRANSLATIONS)
+        transaction.commit()
+        ws_upload = self.load(upload, person)
+
+        self.assertFalse(ws_upload.contains_source)
+        self.assertTrue(ws_upload.contains_build)
+        ws_binaries = ws_upload.getBinaryProperties()
+        self.assertEqual(len(list(bprs)) + 1, len(ws_binaries))
+        for bpr, ws_binary in zip(bprs, ws_binaries):
+            self.assertBinaryPropertiesMatch(arch, bpr, ws_binary)
+        expected_custom = {
+            "name": "foo_%s_%s_translations.tar.gz" % (version, arch),
+            "customformat": "raw-translations",
+            }
+        self.assertEqual(expected_custom, ws_binaries[-1])