launchpad-reviewers team mailing list archive

[Simone Pelosi <mp+489303@xxxxxxxxxxxxxxxxxx>]

Simone Pelosi has proposed merging ~pelpsi/launchpad:extract-component-name-version-should-be-not-just-an-integer into launchpad:master.

Commit message:
Version of a component could be a string

We should support version like _6.8.0-70.70
The previous implementation uses \d+ implying that the version of
a component is an integer.
Extending to (?:_[^_]+) will allow us to support complex component
versions.

LP: #2117415

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #2117415 in Launchpad itself: "snap uploads for snap builds with components which have versions fail"
  https://bugs.launchpad.net/launchpad/+bug/2117415

For more details, see:
https://code.launchpad.net/~pelpsi/launchpad/+git/launchpad/+merge/489303
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pelpsi/launchpad:extract-component-name-version-should-be-not-just-an-integer into launchpad:master.

diff --git a/lib/lp/snappy/model/snapbuildjob.py b/lib/lp/snappy/model/snapbuildjob.py
index 26617a9..a66e000 100644
--- a/lib/lp/snappy/model/snapbuildjob.py
+++ b/lib/lp/snappy/model/snapbuildjob.py
@@ -358,7 +358,7 @@ class SnapStoreUploadJob(SnapBuildJobDerived):
         <component_version> is not mandatory.
         """
 
-        pattern = r"^[^+]+[+](?P<component_name>[^_]+)(?:_\d+)?\.comp$"
+        pattern = r"^[^+]+\+(?P<component_name>[^_]+)(?:_[^_]+)?\.comp$"
 
         match = re.match(pattern, filename)
         if match:
diff --git a/lib/lp/snappy/tests/test_snapbuildjob.py b/lib/lp/snappy/tests/test_snapbuildjob.py
index e9f22a6..8c82b4f 100644
--- a/lib/lp/snappy/tests/test_snapbuildjob.py
+++ b/lib/lp/snappy/tests/test_snapbuildjob.py
@@ -165,7 +165,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         )
         return snapbuild
 
-    def makeSnapBuildWithComponents(self, num_components, use_revision=True):
+    def makeSnapBuildWithComponents(self, num_components, revision="_0"):
         # Make a build with a builder, components, and a webhook.
         snap_name = "test-snap"
         snapcraft_yaml = (
@@ -190,8 +190,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
                 snapbuild=snapbuild, libraryfile=snap_lfa
             )
             filename = "%s+somecomponent%s" % (snap_name, i)
-            if use_revision:
-                filename += "_0"
+            filename += revision
             filename += ".comp"
             component_lfa = self.factory.makeLibraryFileAlias(
                 filename=filename,
@@ -336,7 +335,7 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
         # and then pushes it to the store and records the store URL
         # and revision.
         logger = self.useFixture(FakeLogger())
-        snapbuild = self.makeSnapBuildWithComponents(1, use_revision=False)
+        snapbuild = self.makeSnapBuildWithComponents(1, revision="")
         self.assertContentEqual([], snapbuild.store_upload_jobs)
         job = SnapStoreUploadJob.create(snapbuild)
         client = FakeSnapStoreClient()
@@ -370,6 +369,47 @@ class TestSnapStoreUploadJob(TestCaseWithFactory):
             snapbuild, ["Pending", "Uploaded"], logger
         )
 
+    def test_run_with_component_with_complex_revision(self):
+        # The job uploads the build to the storage with its component
+        # and then pushes it to the store and records the store URL
+        # and revision.
+        logger = self.useFixture(FakeLogger())
+        snapbuild = self.makeSnapBuildWithComponents(
+            1, revision="_6.8.0-70.70"
+        )
+        self.assertContentEqual([], snapbuild.store_upload_jobs)
+        job = SnapStoreUploadJob.create(snapbuild)
+        client = FakeSnapStoreClient()
+        client.uploadFile.result = 1
+        client.push.result = self.status_url
+        client.checkStatus.result = (self.store_url, 1)
+        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
+        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
+            run_isolated_jobs([job])
+
+        # Check if uploadFile is called for snap and its component.
+        self.assertThat(
+            [client.uploadFile.calls[0]], FileUploaded("test-snap_0_all.snap")
+        )
+        self.assertThat(
+            [client.uploadFile.calls[1]],
+            FileUploaded("test-snap+somecomponent0_6.8.0-70.70.comp"),
+        )
+        self.assertEqual(
+            [((snapbuild, 1, {"somecomponent0": 1}), {})],
+            client.push.calls,
+        )
+        self.assertEqual([((self.status_url,), {})], client.checkStatus.calls)
+        self.assertContentEqual([job], snapbuild.store_upload_jobs)
+        self.assertEqual(self.store_url, job.store_url)
+        self.assertEqual(1, job.store_revision)
+        self.assertEqual(1, snapbuild.store_upload_revision)
+        self.assertIsNone(job.error_message)
+        self.assertEqual([], pop_notifications())
+        self.assertWebhookDeliveries(
+            snapbuild, ["Pending", "Uploaded"], logger
+        )
+
     def test_run_with_multiple_components(self):
         # The job uploads the build to the storage with its components
         # and then pushes it to the store and records the store URL