launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/cross-distro-ppa-uploads into lp:launchpad with lp:~wgrant/launchpad/ppa-distro-traversal as a prerequisite.

Commit message:
Support /~person/distro/ppa[/suite] paths in archiveuploader, for non-Ubuntu PPAs. Also drop support for /~person/distro/suite (deprecated five years ago), as it's ambiguous with /~person/distro/ppa.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/cross-distro-ppa-uploads/+merge/225662

Support /~person/distro/ppa[/suite] paths in archiveuploader, for non-Ubuntu PPAs. Also drop support for /~person/distro/suite (deprecated five years ago), as it's ambiguous with /~person/distro/ppa.
-- 
https://code.launchpad.net/~wgrant/launchpad/cross-distro-ppa-uploads/+merge/225662
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/cross-distro-ppa-uploads into lp:launchpad.

=== modified file 'lib/lp/archiveuploader/tests/upload-path-parsing.txt'
--- lib/lp/archiveuploader/tests/upload-path-parsing.txt	2014-07-03 04:51:30 +0000
+++ lib/lp/archiveuploader/tests/upload-path-parsing.txt	2014-07-07 02:47:53 +0000
@@ -78,70 +78,77 @@
 starts with '~', then the subsequent text in this term will be looked
 up as a Person in Launchpad.
 
-    >>> check_upload('~cprov/ppa/ubuntu')
+    >>> from zope.component import getUtility
+    >>> from lp.registry.interfaces.distribution import IDistributionSet
+    >>> from lp.registry.interfaces.person import IPersonSet
+    >>> from lp.soyuz.enums import ArchivePurpose
+    >>> from lp.soyuz.interfaces.archive import IArchiveSet
+    >>> debian = getUtility(IDistributionSet).getByName('debian')
+    >>> cprov = getUtility(IPersonSet).getByName('cprov')
+    >>> copy_archive = getUtility(IArchiveSet).new(
+    ...     ArchivePurpose.PPA, cprov, 'ppa', distribution=debian)
+
+    >>> check_upload('~cprov/ubuntu/ppa')
     Archive: ppa
     Distribution: ubuntu
     Suite: None
 
-    >>> check_upload('~does-not-exist/ppa/ubuntu')
+    >>> check_upload('~cprov/debian/ppa')
+    Archive: ppa
+    Distribution: debian
+    Suite: None
+
+    >>> check_upload('~does-not-exist/ubuntu/ppa')
     Traceback (most recent call last):
     ...
     PPAUploadPathError: Could not find person or team named
     'does-not-exist'.
 
-The second path term is assumed to be a PPA name. Since for a long
-time named-PPA was not supported and uploads didn't need to consider
-it, we still pointing uploads where the PPA name was omitted to the
-default PPA (the one named 'ppa').
-
-    >>> check_upload('~cprov/ubuntu')
+    >>> check_upload('~cprov/notbuntu/ppa')
+    Traceback (most recent call last):
+    ...
+    PPAUploadPathError: Could not find distribution 'notbuntu'.
+
+Two deprecated PPA paths are still supported for compatibility. Before
+mid-2014 all PPAs were for Ubuntu, and the distribution came after the
+PPA name and could be omitted. Before 2009 each user had at most one
+PPA, so the name was omitted -- we assume "ppa".
+
+    >>> check_upload('~cprov/ppa/ubuntu')
     Archive: ppa
     Distribution: ubuntu
     Suite: None
-
-PPA upload paths missing the 'distribution' part default to 'ubuntu'
-distribution.
-
     >>> check_upload('~cprov/ppa')
     Archive: ppa
     Distribution: ubuntu
     Suite: None
 
+    >>> check_upload('~cprov/ubuntu')
+    Archive: ppa
+    Distribution: ubuntu
+    Suite: None
+
 PPA uploads also support overrides to the changesfile suite when it's
-valid. It's also supported for uploads omitting the PPA name.
+valid. It's also supported for uploads to the deprecated paths.
+
+    >>> check_upload('~cprov/ubuntu/ppa/warty-backports')
+    Archive: ppa
+    Distribution: ubuntu
+    Suite: warty-backports
 
     >>> check_upload('~cprov/ppa/ubuntu/warty-backports')
     Archive: ppa
     Distribution: ubuntu
     Suite: warty-backports
 
-    >>> check_upload('~cprov/ubuntu/warty-backports')
-    Archive: ppa
-    Distribution: ubuntu
-    Suite: warty-backports
-
     >>> check_upload('~cprov/ppa/ubuntu/boing')
     Traceback (most recent call last):
     ...
     PPAUploadPathError: Could not find suite 'boing'.
 
-A PPA upload for an conflicting distribution results in an
-error. Currently all PPAs in Launchpad are targeted to 'ubuntu'.
-
-    >>> check_upload('~cprov/ppa/debian')
-    Traceback (most recent call last):
-    ...
-    PPAUploadPathError: PPA for Celso Providelo only supports uploads
-    to 'ubuntu' distribution.
-
 We will disable Celso's default PPA and uploads to it will result in
 an error.
 
-    >>> from zope.component import getUtility
-    >>> from lp.registry.interfaces.distribution import IDistributionSet
-    >>> from lp.registry.interfaces.person import IPersonSet
-
-    >>> cprov = getUtility(IPersonSet).getByName('cprov')
     >>> ubuntu = getUtility(IDistributionSet).getByName('ubuntu')
     >>> cprov_ppa = cprov.getPPAByName(ubuntu, 'ppa')
     >>> cprov_ppa.disable()
@@ -149,32 +156,42 @@
     >>> import transaction
     >>> transaction.commit()
 
+    >>> check_upload('~cprov/ubuntu/ppa')
+    Traceback (most recent call last):
+    ...
+    PPAUploadPathError: PPA for Celso Providelo is disabled.
+
     >>> check_upload('~cprov/ppa/ubuntu')
     Traceback (most recent call last):
     ...
     PPAUploadPathError: PPA for Celso Providelo is disabled.
 
-    >>> check_upload('~cprov/ubuntu')
-    Traceback (most recent call last):
-    ...
-    PPAUploadPathError: PPA for Celso Providelo is disabled.
-
 Uploading to named PPA that does not exist fails.
 
+    >>> check_upload('~cprov/ubuntu/beta')
+    Traceback (most recent call last):
+    ...
+    PPAUploadPathError: Could not find a PPA owned by 'cprov' for
+    'ubuntu' named 'beta'.
+
     >>> check_upload('~cprov/beta/ubuntu')
     Traceback (most recent call last):
     ...
-    PPAUploadPathError: Could not find a PPA named 'beta' for 'cprov'.
+    PPAUploadPathError: Could not find a PPA owned by 'cprov' for
+    'ubuntu' named 'beta'.
 
 We will create a 'beta' PPA for Celso.
 
-    >>> from lp.soyuz.enums import ArchivePurpose
-    >>> from lp.soyuz.interfaces.archive import IArchiveSet
     >>> beta_ppa = getUtility(IArchiveSet).new(
     ...     ArchivePurpose.PPA, cprov, 'beta')
 
 And the upload now found its way to the new named PPA.
 
+    >>> check_upload('~cprov/ubuntu/beta')
+    Archive: beta
+    Distribution: ubuntu
+    Suite: None
+
     >>> check_upload('~cprov/beta/ubuntu')
     Archive: beta
     Distribution: ubuntu
@@ -203,7 +220,14 @@
 
 An extra path part that cannot be processed for PPA uploads.
 
-    >>> check_upload('~cprov/ppa/ubuntu/warty/ding-dong')
+    >>> check_upload('~cprov/ubuntu/ppa/warty/ding-dong')
+    Traceback (most recent call last):
+    ...
+    UploadPathError: Path format mismatch.
+
+A PPA upload missing '~':
+
+    >>> check_upload('cprov/ubuntu/ppa')
     Traceback (most recent call last):
     ...
     UploadPathError: Path format mismatch.
@@ -215,7 +239,7 @@
     ...
     UploadPathError: Could not find distribution 'cprov'.
 
-A named PPA upload missing '~'.
+An old-style named PPA upload missing '~'.
 
     >>> check_upload('cprov/ppa/ubuntu')
     Traceback (most recent call last):

=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py	2014-07-03 04:51:30 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py	2014-07-07 02:47:53 +0000
@@ -57,7 +57,6 @@
 from zope.component import getUtility
 
 from lp.app.errors import NotFoundError
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.archiveuploader.livefsupload import LiveFSUpload
 from lp.archiveuploader.nascentupload import (
     EarlyReturnUploadError,
@@ -784,12 +783,22 @@
     We do this by analysing the path to which the user has uploaded,
     ie. the relative path within the upload folder to the changes file.
 
-    The valid paths are:
-    '' - default distro, ubuntu
-    '<distroname>' - given distribution
-    '~<personname>[/ppa_name]/<distroname>[/distroseriesname]' - given ppa,
-    distribution and optionally a distroseries.  If ppa_name is not
-    specified it will default to the one referenced by IPerson.archive.
+    Current paths are:
+      /<distro>[/suite] - any primary archive
+      /~<person>/<distro>/<ppa>[/suite] - any PPA
+
+    One deprecated form is still supported for the Ubuntu primary archive:
+      / - Ubuntu primary archive
+
+    Three deprecated forms are still supported for Ubuntu PPAs:
+      /~<person>/<ppa>/ubuntu[/suite] - any Ubuntu PPA
+      /~<person>/<ppa> - any Ubuntu PPA
+      /~<person>/ubuntu - Ubuntu PPA named "ppa"
+
+    The original /~<person>/ubuntu/<suite> form is no longer supported
+    as it clashes with /~<person>/<distro>/<ppa>
+
+    suite is an optional distroseries with an optional pocket suffix.
 
     I raises UploadPathError if something was wrong when parsing it.
 
@@ -802,7 +811,7 @@
 
     if (not first_path.startswith('~') and not first_path.isdigit()
         and len(parts) <= 2):
-        # Distribution upload (<distro>[/distroseries]). Always targeted to
+        # Distribution upload (<distro>[/suite]). Always targeted to
         # the corresponding primary archive.
         distribution, suite_name = _getDistributionAndSuite(
             parts, UploadPathError)
@@ -811,7 +820,13 @@
     elif (first_path.startswith('~') and
           len(parts) >= 2 and
           len(parts) <= 4):
-        # PPA upload (~<person>[/ppa_name]/<distro>[/distroseries]).
+        # PPA uploads look like (~<person>/<distro>/<ppa_name>[/suite]).
+        #
+        # Additionally, three deprecated formats are still supported for
+        # Ubuntu PPAs:
+        #  (~<person>/<ppa_name>/ubuntu[/suite])
+        #  (~<person>/<ppa_name>)
+        #  (~<person>/ubuntu)
 
         # Skip over '~' from the person name.
         person_name = first_path[1:]
@@ -820,26 +835,35 @@
             raise PPAUploadPathError(
                 "Could not find person or team named '%s'." % person_name)
 
-        ppa_name = parts[1]
-
-        # Compatibilty feature for allowing unamed-PPA upload paths
-        # for a certain period of time.
-        if ppa_name == 'ubuntu':
-            ppa_name = 'ppa'
-            distribution_and_suite = parts[1:]
+        if len(parts) == 2:
+            distro_name = 'ubuntu'
+            suite = []
+            if parts[1] == 'ubuntu':
+                # Old nameless path.
+                ppa_name = 'ppa'
+            else:
+                # Old named but distroless path.
+                ppa_name = parts[1]
         else:
-            distribution_and_suite = parts[2:]
+            if parts[2] == 'ubuntu':
+                # Old named path with PPA name before distro.
+                distro_name = 'ubuntu'
+                ppa_name = parts[1]
+            else:
+                # Modern path with distro, name, suite.
+                distro_name = parts[1]
+                ppa_name = parts[2]
+            suite = parts[3:]
+
+        distribution, suite_name = _getDistributionAndSuite(
+            [distro_name] + suite, PPAUploadPathError)
 
         try:
-            archive = person.getPPAByName(
-                getUtility(ILaunchpadCelebrities).ubuntu, ppa_name)
+            archive = person.getPPAByName(distribution, ppa_name)
         except NoSuchPPA:
             raise PPAUploadPathError(
-                "Could not find a PPA named '%s' for '%s'."
-                % (ppa_name, person_name))
-
-        distribution, suite_name = _getDistributionAndSuite(
-            distribution_and_suite, PPAUploadPathError)
+                "Could not find a PPA owned by '%s' for '%s' named '%s'."
+                % (person.name, distribution.name, ppa_name))
 
     elif first_path.isdigit():
         # This must be a binary upload from a build slave.