launchpad-reviewers team mailing list archive

[Tushar Gupta <mp+480702@xxxxxxxxxxxxxxxxxx>]

Tushar Gupta has proposed merging ~tushar5526/launchpad:bug-fixes-and-optimisations-for-archive-and-exclude-options into launchpad:master.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~tushar5526/launchpad/+git/launchpad/+merge/480702
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~tushar5526/launchpad:bug-fixes-and-optimisations-for-archive-and-exclude-options into launchpad:master.

diff --git a/lib/lp/archivepublisher/scripts/publishdistro.py b/lib/lp/archivepublisher/scripts/publishdistro.py
index 33982d8..fe9e002 100644
--- a/lib/lp/archivepublisher/scripts/publishdistro.py
+++ b/lib/lp/archivepublisher/scripts/publishdistro.py
@@ -646,10 +646,8 @@ class PublishDistro(PublisherScript):
         exclude_options = []
         # If there are any archives specified to be excluded, exclude rsync
         # for them in the rsync command
-        for excluded_archive in self.findArchives(
-            self.options.excluded_archives
-        ):
-            exclude_options.extend(["--exclude", excluded_archive.reference])
+        for excluded_archive_reference in self.options.excluded_archives:
+            exclude_options.extend(["--exclude", excluded_archive_reference])
         return [
             self._buildRsyncCommand(
                 extra_options=exclude_options,
@@ -687,14 +685,6 @@ class PublishDistro(PublisherScript):
                 archive = archive_set.getPPAByDistributionAndOwnerName(
                     distribution, owner_path.name[1:], archive_path.name
                 )
-
-                # If --archive is set, run only for the specified archives
-                # and skip others.
-                if (
-                    self.options.archives
-                    and archive.reference not in self.options.archives
-                ):
-                    continue
                 if archive is None:
                     self.logger.info(
                         "Skipping OVAL data for '~%s/%s/%s' "
@@ -704,11 +694,16 @@ class PublishDistro(PublisherScript):
                         archive_path.name,
                     )
                     continue
-                # skip any archive excluded by --exclude
-                if archive in self.findArchives(
-                    self.options.excluded_archives, distribution
+                # If --archive is set, run only for the specified archives
+                # and skip others.
+                if (
+                    self.options.archives
+                    and archive.reference not in self.options.archives
                 ):
                     continue
+                # skip any archive excluded by --exclude
+                if archive.reference in self.options.excluded_archives:
+                    continue
                 for suite_path in archive_path.iterdir():
                     try:
                         series, pocket = distribution.getDistroSeriesAndPocket(
diff --git a/lib/lp/archivepublisher/tests/test_publishdistro.py b/lib/lp/archivepublisher/tests/test_publishdistro.py
index f5f71e9..f615f9a 100644
--- a/lib/lp/archivepublisher/tests/test_publishdistro.py
+++ b/lib/lp/archivepublisher/tests/test_publishdistro.py
@@ -345,12 +345,21 @@ class TestPublishDistro(TestNativePublishingBase):
         self.setUpOVALDataRsync()
         ppa1 = self.factory.makeArchive(private=True)
         ppa2 = self.factory.makeArchive()
+        non_existing_ppa_reference = "~foo/bar/ppa"
         self.factory.makeArchive()
 
         mock_subprocess_check_call = self.useFixture(
             MockPatch("lp.archivepublisher.scripts.publishdistro.check_call")
         ).mock
 
+        exclude_options = [
+            "--exclude",
+            ppa1.reference,
+            "--exclude",
+            ppa2.reference,
+            "--exclude",
+            non_existing_ppa_reference,
+        ]
         call_args = [
             "/usr/bin/rsync",
             "-a",
@@ -358,21 +367,18 @@ class TestPublishDistro(TestNativePublishingBase):
             "--timeout=90",
             "--delete",
             "--delete-after",
-            "--exclude",
-            ppa1.reference,
-            "--exclude",
-            ppa2.reference,
-            "oval.internal::oval/",
-            self.oval_data_root + "/",
         ]
-        self.runPublishDistro(
-            extra_args=[
-                "--exclude",
-                ppa1.reference,
-                "--exclude",
-                ppa2.reference,
+        call_args.extend(exclude_options)
+
+        call_args.extend(
+            [
+                "oval.internal::oval/",
+                self.oval_data_root + "/",
             ]
         )
+        self.runPublishDistro(
+            extra_args=exclude_options,
+        )
         mock_subprocess_check_call.assert_called_once_with(call_args)
 
     def testPublishDistroOVALDataRsyncForSpecificArchives(self):
@@ -610,12 +616,13 @@ class TestPublishDistro(TestNativePublishingBase):
         self.assertIsNone(ppa2.dirty_suites)
         self.assertIsNone(ppa3.dirty_suites)
 
-    def test_checkForUpdatedOVALData_runs_for_specific_archive(self):
+    def test_checkForUpdatedOVALData_for_specific_archive(self):
         """
         checkForUpdatedOVALData should only run for specific archives
         if "archive" option is specified.
         """
 
+        distribution = "ubuntu"
         self.setUpOVALDataRsync()
         self.useFixture(
             MockPatch("lp.archivepublisher.scripts.publishdistro.check_call")
@@ -624,12 +631,21 @@ class TestPublishDistro(TestNativePublishingBase):
         ppa1 = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
         ppa2 = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
         ppa3 = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PPA)
+        # another ppa with different distribution
+        ppa4 = self.factory.makeArchive(
+            purpose=ArchivePurpose.PPA,
+            distribution=getUtility(IDistributionSet)["ubuntutest"],
+        )
+
+        non_existing_ppa_reference = "~foo/bar/ppa"
+
         # Disable normal publication so that dirty_suites isn't cleared.
         ppa1.publish = False
         ppa2.publish = False
         ppa3.publish = False
 
-        for archive in [ppa1, ppa2, ppa3]:
+        for archive in [ppa1, ppa2, ppa3, ppa4]:
             incoming_dir = (
                 Path(self.oval_data_root)
                 / archive.reference
@@ -638,19 +654,48 @@ class TestPublishDistro(TestNativePublishingBase):
             )
             write_file(str(incoming_dir), b"test")
 
+        # test for code paths when there is an non-existent archive dir
+        # present in oval_data_root
+        nonexistent_archive_dir = (
+            Path(self.oval_data_root)
+            / "~nonexistent"
+            / distribution
+            / "archive"
+            / "breezy-autotest"
+            / "main"
+        )
+        write_file(str(nonexistent_archive_dir / "foo.oval.xml.bz2"), b"test")
+
+        # test for code paths when there is an non-existent suite dir
+        # present in oval_data_root
+        nonexistent_suite_dir = (
+            Path(self.oval_data_root)
+            / archive.reference
+            / "nonexistent"
+            / "main"
+        )
+
+        write_file(str(nonexistent_suite_dir / "foo.oval.xml.bz2"), b"test")
         self.runPublishDistro(
             extra_args=[
                 "--archive",
                 ppa1.reference,
                 "--archive",
                 ppa2.reference,
+                "--archive",
+                ppa4.reference,
+                "--archive",
+                non_existing_ppa_reference,
             ],
-            distribution="ubuntu",
+            distribution=distribution,
         )
 
         self.assertEqual(["breezy-autotest"], ppa1.dirty_suites)
         self.assertEqual(["breezy-autotest"], ppa2.dirty_suites)
         self.assertIsNone(ppa3.dirty_suites)
+        # ppa4 has different distribution than the target distribution so
+        # it should be skipped.
+        self.assertIsNone(ppa4.dirty_suites)
 
         # Further logs should not have any reference to other PPAs
         # as we skip them when --archive option is set.