launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #13658
[Merge] lp:~cjwatson/launchpad/redirect-release-uploads into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/redirect-release-uploads into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1068071 in Launchpad itself: "Need facility to redirect Ubuntu uploads to -proposed pocket"
https://bugs.launchpad.net/launchpad/+bug/1068071
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/redirect-release-uploads/+merge/131155
== Summary ==
''Summarise the problem that you're solving.''
Bug 1068071: To make raring more continuously usable, UE wants to land all uploads in raring-proposed and promote them automatically to raring following tests somewhat akin to those used by Debian's testing suite, in what amounts to a continuous integration system. We'll operate the automation, but its usefulness will be significantly enhanced by redirecting uploads automatically from raring to raring-proposed.
== Proposed fix ==
A new Distribution.redirect_release_uploads column is making its way through buildbot/QA as I type this. If it's set, then:
* Automatically rewrite source uploads from RELEASE to PROPOSED in archiveuploader, adding a note to acceptance mails that this has happened.
* Forbid copies into the RELEASE pocket, except for queue admins (if nothing else, such copies are used by the automation itself).
== Pre-implementation notes ==
Extensive discussions on #ubuntu-release over the last week or so. I was going to automatically rewrite some copies as well, but William convinced me that this was excessively magical and complicated the API in ways that would be troublesome in future. We'll probably end up SRUing syncpackage instead.
== Implementation details ==
The insecure upload policy seemed a sensible place to do redirects, but it does have the slight problem that setDistroSeriesAndPocket is sometimes called before we have a NascentUpload to add warnings to. I ended up stashing the warning temporarily in policy.redirect_warning, which is a little bit of a kludge.
This causes acceptance mails to have some extra bits (exemplified by the changes to nascentupload-announcements.txt) due to the normal contents of %(SUMMARY)s. I don't think this is a problem.
The copy restriction seemed most naturally done in checkUploadToPocket, although that needs to be guarded by a check_redirect=False parameter because that method is also called for build uploads which must not be redirected. A couple of places were doing their own checks for copies into the RELEASE pocket of PPAs; I couldn't see any reason why these wouldn't also want all the other checks in checkUploadToPocket - indeed, copies into the pockets forbidden there will generally cause publisher badness - other than making sure that they raised the correct exception type, so I rewrote these in terms of that method.
== LOC Rationale ==
+139. This seems cheap at the price for what should be a major improvement in the ongoing stability of Ubuntu. I have about 5600 lines of credit to help absorb this, and still have some more refactoring work to come.
== Tests ==
Probably wants the whole test suite, but in particular:
bin/test -vvct archiveuploader -t soyuz
== Demo and Q/A ==
Flip the switch on dogfood, upload source to the RELEASE pocket, whatever its current development series is, check that it goes to PROPOSED, check that a non-queue-admin can't copy it into RELEASE, and check that a queue admin can.
== Lint ==
Pre-existing / false positives:
./lib/lp/archiveuploader/tests/nascentupload-announcements.txt
186: want exceeds 78 characters.
187: want exceeds 78 characters.
660: want exceeds 78 characters.
727: want exceeds 78 characters.
729: want exceeds 78 characters.
780: want exceeds 78 characters.
782: want exceeds 78 characters.
./lib/lp/soyuz/emailtemplates/upload-accepted.txt
14: Line has trailing whitespace.
./lib/lp/soyuz/scripts/packagecopier.py
50: E302 expected 2 blank lines, found 1
--
https://code.launchpad.net/~cjwatson/launchpad/redirect-release-uploads/+merge/131155
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/redirect-release-uploads into lp:launchpad.
=== modified file 'lib/lp/archiveuploader/nascentupload.py'
--- lib/lp/archiveuploader/nascentupload.py 2012-07-03 08:04:35 +0000
+++ lib/lp/archiveuploader/nascentupload.py 2012-10-24 10:04:35 +0000
@@ -138,6 +138,9 @@
self.reject(
"Unable to find distroseries: %s" % self.changes.suite_name)
+ if policy.redirect_warning is not None:
+ self.warn(policy.redirect_warning)
+
# Make sure the changes file name is well-formed.
self.run_and_reject_on_error(self.changes.checkFileName)
=== modified file 'lib/lp/archiveuploader/tests/nascentupload-announcements.txt'
--- lib/lp/archiveuploader/tests/nascentupload-announcements.txt 2012-03-29 07:56:15 +0000
+++ lib/lp/archiveuploader/tests/nascentupload-announcements.txt 2012-10-24 10:04:35 +0000
@@ -513,6 +513,11 @@
DEBUG
DEBUG ==
DEBUG
+ DEBUG OK: bar_1.0.orig.tar.gz
+ DEBUG OK: bar_1.0-4.diff.gz
+ DEBUG OK: bar_1.0-4.dsc
+ DEBUG -> Component: universe Section: devel
+ DEBUG
DEBUG Announcing to hoary-announce@xxxxxxxxxxxxxxxx
DEBUG
DEBUG Thank you for your contribution to Ubuntu Linux.
@@ -728,6 +733,11 @@
<BLANKLINE>
=3D=3D
<BLANKLINE>
+ OK: bar_1.0.orig.tar.gz
+ OK: bar_1.0-10.diff.gz
+ OK: bar_1.0-10.dsc
+ -> Component: universe Section: devel
+ <BLANKLINE>
Announcing to hoary-announce@xxxxxxxxxxxxxxxx
<BLANKLINE>
Thank you for your contribution to Ubuntu Linux.
=== modified file 'lib/lp/archiveuploader/tests/test_sync_notification.py'
--- lib/lp/archiveuploader/tests/test_sync_notification.py 2012-01-06 06:07:34 +0000
+++ lib/lp/archiveuploader/tests/test_sync_notification.py 2012-10-24 10:04:35 +0000
@@ -38,6 +38,7 @@
self.distroseries = spph.distroseries
self.archive = spph.distroseries.main_archive
self.pocket = spph.pocket
+ self.redirect_warning = None
setDistroSeriesAndPocket = FakeMethod()
validateUploadType = FakeMethod()
=== modified file 'lib/lp/archiveuploader/tests/test_uploadpolicy.py'
--- lib/lp/archiveuploader/tests/test_uploadpolicy.py 2012-09-13 05:14:05 +0000
+++ lib/lp/archiveuploader/tests/test_uploadpolicy.py 2012-10-24 10:04:35 +0000
@@ -16,6 +16,7 @@
InsecureUploadPolicy,
)
from lp.registry.interfaces.distribution import IDistributionSet
+from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.registry.interfaces.series import SeriesStatus
from lp.services.database.sqlbase import flush_database_updates
from lp.testing import (
@@ -193,6 +194,20 @@
NotFoundError, policy.setDistroSeriesAndPocket,
'nonexistent_security')
+ def test_redirect_release_uploads(self):
+ # With the insecure policy, the
+ # Distribution.redirect_release_uploads flag causes uploads to the
+ # RELEASE pocket to be automatically redirected to PROPOSED.
+ ubuntu = getUtility(IDistributionSet)["ubuntu"]
+ with celebrity_logged_in("admin"):
+ ubuntu.redirect_release_uploads = True
+ flush_database_updates()
+ insecure_policy = findPolicyByName("insecure")
+ insecure_policy.setOptions(FakeOptions(distroseries="hoary"))
+ self.assertEqual("hoary", insecure_policy.distroseries.name)
+ self.assertEqual(
+ PackagePublishingPocket.PROPOSED, insecure_policy.pocket)
+
def setHoaryStatus(self, status):
ubuntu = getUtility(IDistributionSet)["ubuntu"]
with celebrity_logged_in("admin"):
=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py 2012-09-27 02:53:00 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py 2012-10-24 10:04:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Functional tests for uploadprocessor.py."""
@@ -1963,6 +1963,26 @@
self.assertEqual("optional", ddeb.priority_name)
self.assertEqual(deb.priority_name, ddeb.priority_name)
+ def test_redirect_release_uploads(self):
+ # Setting the Distribution.redirect_release_uploads flag causes
+ # release pocket uploads to be redirected to proposed.
+ uploadprocessor = self.setupBreezyAndGetUploadProcessor(
+ policy="insecure")
+ self.switchToAdmin()
+ self.ubuntu.redirect_release_uploads = True
+ self.switchToUploader()
+ upload_dir = self.queueUpload("bar_1.0-1")
+ self.processUpload(uploadprocessor, upload_dir)
+ _, _, raw_msg = stub.test_emails.pop()
+ msg = message_from_string(raw_msg)
+ body = msg.get_payload(0).get_payload(decode=True)
+ self.assertIn(
+ "Redirecting ubuntu breezy to ubuntu breezy-proposed.", body)
+ [queue_item] = self.breezy.getPackageUploads(
+ status=PackageUploadStatus.NEW, name=u"bar",
+ version=u"1.0-1", exact_match=True)
+ self.assertEqual(PackagePublishingPocket.PROPOSED, queue_item.pocket)
+
class TestUploadHandler(TestUploadProcessorBase):
=== modified file 'lib/lp/archiveuploader/uploadpolicy.py'
--- lib/lp/archiveuploader/uploadpolicy.py 2012-09-17 05:25:38 +0000
+++ lib/lp/archiveuploader/uploadpolicy.py 2012-10-24 10:04:35 +0000
@@ -76,6 +76,7 @@
name = 'abstract'
options = None
accepted_type = None # Must be defined in subclasses.
+ redirect_warning = None
def __init__(self):
"""Prepare a policy..."""
@@ -201,6 +202,19 @@
name = 'insecure'
accepted_type = ArchiveUploadType.SOURCE_ONLY
+ def setDistroSeriesAndPocket(self, dr_name):
+ """Set the distroseries and pocket from the provided name.
+
+ The insecure policy redirects uploads to a different pocket if
+ Distribution.redirect_release_uploads is set.
+ """
+ super(InsecureUploadPolicy, self).setDistroSeriesAndPocket(dr_name)
+ if (self.distro.redirect_release_uploads and
+ self.pocket == PackagePublishingPocket.RELEASE):
+ self.pocket = PackagePublishingPocket.PROPOSED
+ self.redirect_warning = "Redirecting %s to %s-proposed." % (
+ self.distroseries, self.distroseries)
+
def rejectPPAUploads(self, upload):
"""Insecure policy allows PPA upload."""
return False
=== modified file 'lib/lp/archiveuploader/uploadprocessor.py'
--- lib/lp/archiveuploader/uploadprocessor.py 2012-06-29 08:40:05 +0000
+++ lib/lp/archiveuploader/uploadprocessor.py 2012-10-24 10:04:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Code for 'processing' 'uploads'. Also see nascentupload.py.
@@ -385,6 +385,9 @@
str(e)))
return UploadStatusEnum.REJECTED
+ if policy.redirect_warning is not None:
+ upload.warn(policy.redirect_warning)
+
# Reject source upload to buildd upload paths.
first_path = relative_path.split(os.path.sep)[0]
if (first_path.isdigit() and
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2012-10-22 10:09:48 +0000
+++ lib/lp/registry/configure.zcml 2012-10-24 10:04:35 +0000
@@ -1623,6 +1623,7 @@
official_malone
owner
package_derivatives_email
+ redirect_release_uploads
summary
title"/>
<require
=== modified file 'lib/lp/registry/interfaces/distribution.py'
--- lib/lp/registry/interfaces/distribution.py 2012-10-05 05:34:13 +0000
+++ lib/lp/registry/interfaces/distribution.py 2012-10-24 10:04:35 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Interfaces including and related to IDistribution."""
@@ -361,6 +361,11 @@
description=_("True if this distribution has sources published."),
readonly=True, required=False)
+ redirect_release_uploads = exported(Bool(
+ title=_("Redirect release pocket uploads"),
+ description=_("Redirect release pocket uploads to proposed pocket"),
+ readonly=False, required=True))
+
def getArchiveIDList(archive=None):
"""Return a list of archive IDs suitable for sqlvalues() or quote().
=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py 2012-09-27 18:04:35 +0000
+++ lib/lp/registry/model/distribution.py 2012-10-24 10:04:35 +0000
@@ -257,6 +257,7 @@
schema=TranslationPermission, default=TranslationPermission.OPEN)
active = True
package_derivatives_email = StringCol(notNull=False, default=None)
+ redirect_release_uploads = BoolCol(notNull=True, default=False)
def __repr__(self):
displayname = self.displayname.encode('ASCII', 'backslashreplace')
=== modified file 'lib/lp/soyuz/doc/archive.txt'
--- lib/lp/soyuz/doc/archive.txt 2012-09-27 02:53:00 +0000
+++ lib/lp/soyuz/doc/archive.txt 2012-10-24 10:04:35 +0000
@@ -2175,7 +2175,7 @@
... "package3", "1.2", cprov.archive, "updates", person=mark)
Traceback (most recent call last):
...
- CannotCopy: Destination pocket must be 'release' for a PPA.
+ CannotCopy: PPA uploads must be for the RELEASE pocket.
syncSource() will always use only the latest publication of the
specific source, ignoring the previous ones. Multiple publications can
=== modified file 'lib/lp/soyuz/doc/soyuz-set-of-uploads.txt'
--- lib/lp/soyuz/doc/soyuz-set-of-uploads.txt 2012-10-03 08:20:24 +0000
+++ lib/lp/soyuz/doc/soyuz-set-of-uploads.txt 2012-10-24 10:04:35 +0000
@@ -74,7 +74,6 @@
Having set up that infrastructure we need to prepare a breezy distroseries
for the ubuntutest distribution.
- >>> from lp.registry.interfaces.pocket import PackagePublishingPocket
>>> from lp.registry.model.distribution import Distribution
>>> from lp.soyuz.enums import PackageUploadStatus
>>> from lp.soyuz.scripts.initialize_distroseries import (
@@ -101,7 +100,9 @@
Add disk content for file inherited from ubuntu/breezy-autotest:
- >>> from lp.services.librarianserver.testing.server import fillLibrarianFile
+ >>> from lp.services.librarianserver.testing.server import (
+ ... fillLibrarianFile,
+ ... )
>>> fillLibrarianFile(54)
Now that the infrastructure is ready, we prepare a set of useful methods.
@@ -345,6 +346,11 @@
<BLANKLINE>
=3D=3D
<BLANKLINE>
+ OK: bar_1.0.orig.tar.gz
+ OK: bar_1.0-4.diff.gz
+ OK: bar_1.0-4.dsc
+ -> Component: universe Section: devel
+ <BLANKLINE>
Announcing to breezy-changes@xxxxxxxxxx
<BLANKLINE>
Thank you for your contribution to Ubuntu Test.
=== modified file 'lib/lp/soyuz/emailtemplates/upload-accepted.txt'
--- lib/lp/soyuz/emailtemplates/upload-accepted.txt 2011-12-18 23:30:56 +0000
+++ lib/lp/soyuz/emailtemplates/upload-accepted.txt 2012-10-24 10:04:35 +0000
@@ -5,6 +5,8 @@
==
+%(SUMMARY)s
+
%(ANNOUNCE)s
Thank you for your contribution to %(DISTRO)s.
=== modified file 'lib/lp/soyuz/interfaces/archive.py'
--- lib/lp/soyuz/interfaces/archive.py 2012-10-23 12:36:50 +0000
+++ lib/lp/soyuz/interfaces/archive.py 2012-10-24 10:04:35 +0000
@@ -42,6 +42,7 @@
'NoTokensForTeams',
'PocketNotFound',
'PriorityNotFound',
+ 'RedirectedPocket',
'SectionNotFound',
'VersionRequiresName',
'default_name_by_purpose',
@@ -206,6 +207,19 @@
"'%s' state." % (pocket.name, distroseries.status.name))
+@error_status(httplib.FORBIDDEN)
+class RedirectedPocket(Exception):
+ """Returned for a pocket that would normally be redirected to another.
+
+ This is used in contexts (e.g. copies) where actually doing the
+ redirection would be Too Much Magic."""
+
+ def __init__(self, distroseries, pocket, preferred):
+ Exception.__init__(self,
+ "Not permitted to upload directly to %s; try %s instead." %
+ (distroseries.getSuite(pocket), distroseries.getSuite(preferred)))
+
+
class CannotUploadToPPA(CannotUploadToArchive):
"""Raised when a person cannot upload to a PPA."""
=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py 2012-10-24 09:25:16 +0000
+++ lib/lp/soyuz/model/archive.py 2012-10-24 10:04:35 +0000
@@ -148,6 +148,7 @@
NoSuchPPA,
NoTokensForTeams,
PocketNotFound,
+ RedirectedPocket,
validate_external_dependencies,
VersionRequiresName,
)
@@ -1266,7 +1267,7 @@
# Allow anything else.
return True
- def checkUploadToPocket(self, distroseries, pocket):
+ def checkUploadToPocket(self, distroseries, pocket, check_redirect=False):
"""See `IArchive`."""
if self.is_partner:
if pocket not in (
@@ -1282,6 +1283,11 @@
# existing builds after a series is released.
return
else:
+ if (check_redirect and
+ pocket == PackagePublishingPocket.RELEASE and
+ self.distribution.redirect_release_uploads):
+ return RedirectedPocket(
+ distroseries, pocket, PackagePublishingPocket.PROPOSED)
# Uploads to the partner archive are allowed in any distroseries
# state.
# XXX julian 2005-05-29 bug=117557:
@@ -1799,14 +1805,11 @@
from lp.soyuz.scripts.packagecopier import do_copy
pocket = self._text_to_pocket(to_pocket)
- # Fail immediately if the destination pocket is not Release and
- # this archive is a PPA.
- if self.is_ppa and pocket != PackagePublishingPocket.RELEASE:
- raise CannotCopy(
- "Destination pocket must be 'release' for a PPA.")
-
- # Now convert the to_series string to a real distroseries.
series = self._text_to_series(to_series)
+ reason = self.checkUploadToPocket(series, pocket, check_redirect=True)
+ if reason:
+ # Wrap any forbidden-pocket error in CannotCopy.
+ raise CannotCopy(unicode(reason))
# Perform the copy, may raise CannotCopy. Don't do any further
# permission checking: this method is protected by
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2012-10-23 12:36:50 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2012-10-24 10:04:35 +0000
@@ -611,10 +611,11 @@
:raise CannotCopy: If the copy fails for a reason that the user
can deal with.
"""
- if self.target_archive.is_ppa:
- if self.target_pocket != PackagePublishingPocket.RELEASE:
- raise CannotCopy(
- "Destination pocket must be 'release' for a PPA.")
+ reason = self.target_archive.checkUploadToPocket(
+ self.target_distroseries, self.target_pocket, check_redirect=True)
+ if reason:
+ # Wrap any forbidden-pocket error in CannotCopy.
+ raise CannotCopy(unicode(reason))
source_package = self.findSourcePublication()
=== modified file 'lib/lp/soyuz/scripts/packagecopier.py'
--- lib/lp/soyuz/scripts/packagecopier.py 2012-09-28 06:15:58 +0000
+++ lib/lp/soyuz/scripts/packagecopier.py 2012-10-24 10:04:35 +0000
@@ -205,7 +205,8 @@
destination_component = None
# Is the destination pocket open at all?
- reason = archive.checkUploadToPocket(dest_series, pocket)
+ reason = archive.checkUploadToPocket(
+ dest_series, pocket, check_redirect=True)
if reason is not None:
raise CannotCopy(reason)
=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py 2012-10-24 09:25:16 +0000
+++ lib/lp/soyuz/tests/test_archive.py 2012-10-24 10:04:35 +0000
@@ -66,6 +66,7 @@
NoRightsForArchive,
NoRightsForComponent,
NoSuchPPA,
+ RedirectedPocket,
VersionRequiresName,
)
from lp.soyuz.interfaces.archivearch import IArchiveArchSet
@@ -713,6 +714,22 @@
archive.checkUploadToPocket(
distroseries, PackagePublishingPocket.RELEASE))
+ def test_checkUploadToPocket_handles_redirects(self):
+ # Uploading to the release pocket is disallowed if
+ # Distribution.redirect_release_uploads is set.
+ archive, distroseries = self.makeArchiveAndActiveDistroSeries(
+ purpose=ArchivePurpose.PRIMARY)
+ with person_logged_in(archive.distribution.owner):
+ archive.distribution.redirect_release_uploads = True
+ self.assertIsInstance(
+ archive.checkUploadToPocket(
+ distroseries, PackagePublishingPocket.RELEASE,
+ check_redirect=True),
+ RedirectedPocket)
+ self.assertIsNone(
+ archive.checkUploadToPocket(
+ distroseries, PackagePublishingPocket.PROPOSED))
+
def test_checkUpload_package_permission(self):
archive, distroseries = self.makeArchiveAndActiveDistroSeries(
purpose=ArchivePurpose.PRIMARY)
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2012-10-23 12:36:50 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2012-10-24 10:04:35 +0000
@@ -416,8 +416,7 @@
self.assertEqual(JobStatus.FAILED, job.status)
self.assertEqual(
- "Destination pocket must be 'release' for a PPA.",
- job.error_message)
+ "PPA uploads must be for the RELEASE pocket.", job.error_message)
def assertOopsRecorded(self, job):
self.assertEqual(JobStatus.FAILED, job.status)
@@ -451,6 +450,29 @@
transaction.abort()
self.assertOopsRecorded(job)
+ def test_target_primary_redirects(self):
+ # For primary archives with redirect_release_uploads set, ordinary
+ # uploaders may not copy directly into the release pocket.
+ job = create_proper_job(self.factory)
+ job.target_archive.distribution.redirect_release_uploads = True
+ naked_job = removeSecurityProxy(job)
+ naked_job.reportFailure = FakeMethod()
+ transaction.commit()
+ # CannotCopy exceptions when copying into a primary archive are
+ # swallowed, but reportFailure is still called.
+ self.runJob(job)
+ self.assertEqual(1, naked_job.reportFailure.call_count)
+
+ def test_target_primary_queue_admin_bypasses_redirect(self):
+ # For primary archives with redirect_release_uploads set, queue
+ # admins may copy directly into the release pocket anyway.
+ job = create_proper_job(self.factory)
+ job.target_archive.distribution.redirect_release_uploads = True
+ with person_logged_in(job.target_archive.owner):
+ job.target_archive.newPocketQueueAdmin(
+ job.requester, PackagePublishingPocket.RELEASE)
+ self.runJob(job)
+
def test_run(self):
# A proper test run synchronizes packages.
Follow ups
