launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04277
[Merge] lp:~jtv/launchpad/bug-809841 into lp:launchpad
Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-809841 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #809841 in Launchpad itself: ""All Ubuntu-derived distros" option for process-accepted"
https://bugs.launchpad.net/launchpad/+bug/809841
For more details, see:
https://code.launchpad.net/~jtv/launchpad/bug-809841/+merge/67994
= Summary =
The process-accepted script accepts (and requires) one non-option argument: the distro name to operate on.
In order to support any number of Ubuntu-derived distributions in Launchpad, we are giving it a new option, --derived, that goes through all those derived distros at once.
== Proposed fix ==
Isolate the "pick a distro" code in a method, which returns a list of distros. From that point on, the script became unaware of the difference between operating on one distro or operating on multiple. There is no "self.distribution" to confuse things.
The TargetPolicy for the derived distros is still DistroTargetPolicy: this mode selects the same kinds of archives as a regular single-distro run and describes them in the same way to the user.
== Pre-implementation notes ==
Discussed with Julian. Ubuntu itself is too big, so we treat that specially using the classic "name the distro on the command line" mode.
We may come up with a smarter way to draw this distinction if other derived distros get similarly large. For now, all derived distros are expected to have overlay relationships to their parent distros, and so never copy the full bulk of the Ubuntu packages into their own archives.
== Implementation details ==
Ubuntu and its derived distros are the only distributions we are capable of publishing, and we're not expecting to see any other derived distros. So the logic for --derived is quite simply: find all derived distributions that are not Ubuntu.
I haven't gone into failure modes. There are some intricacies, but I don't know how rigidly they are specified. In particular, bugs associated with successfully processed queue items are closed afterwards; I chose to do this once for every distribution, rather than saving them all up until the end.
This will behave most sensibly if the script is interrupted, I think: if there are a thousand derived distros, and the last one fails, all the other distros will still have the resolved bugs closed.
== Tests ==
{{{
./bin/test -vvc lp.soyuz.tests.test_processaccepted
}}}
== Demo and Q/A ==
The process-accepted script should still work normally. With the --derived option (and no distribution name), it will loop over all derived distros' archives and series and process all those uploads.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/soyuz/scripts/processaccepted.py
lib/lp/soyuz/tests/test_processaccepted.py
--
https://code.launchpad.net/~jtv/launchpad/bug-809841/+merge/67994
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-809841 into lp:launchpad.
=== modified file 'lib/lp/soyuz/scripts/processaccepted.py'
--- lib/lp/soyuz/scripts/processaccepted.py 2011-07-13 14:32:18 +0000
+++ lib/lp/soyuz/scripts/processaccepted.py 2011-07-14 17:06:45 +0000
@@ -19,6 +19,7 @@
from zope.component import getUtility
from zope.security.proxy import removeSecurityProxy
+from canonical.launchpad.interfaces.lpstorm import IStore
from canonical.launchpad.webapp.errorlog import (
ErrorReportingUtility,
ScriptRequest,
@@ -30,6 +31,7 @@
from lp.bugs.interfaces.bugtask import BugTaskStatus
from lp.registry.interfaces.distribution import IDistributionSet
from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.model.distroseriesparent import DistroSeriesParent
from lp.services.scripts.base import (
LaunchpadCronScript,
LaunchpadScriptFailure,
@@ -260,8 +262,8 @@
help="Whether to treat this as a dry-run or not.")
self.parser.add_option(
- '--derived', action="store_true", dest="derived", default=False,
- help="Process all Ubuntu-derived distributions.")
+ '-D', '--derived', action="store_true", dest="derived",
+ default=False, help="Process all Ubuntu-derived distributions.")
self.parser.add_option(
"--ppa", action="store_true",
@@ -285,24 +287,49 @@
else:
self.txn.commit()
- def findTargetDistribution(self):
- distro_name = self.args[0]
- self.logger.debug("Finding distribution %s." % distro_name)
- distribution = getUtility(IDistributionSet).getByName(distro_name)
- if distribution is None:
+ def findDerivedDistros(self):
+ """Find Ubuntu-derived distributions."""
+ # Avoid circular imports.
+ from lp.registry.model.distribution import Distribution
+ from lp.registry.model.distroseries import DistroSeries
+
+ ubuntu_id = getUtility(ILaunchpadCelebrities).ubuntu.id
+ return IStore(DistroSeries).find(
+ Distribution,
+ Distribution.id == DistroSeries.distributionID,
+ DistroSeries.id == DistroSeriesParent.derived_series_id,
+ DistroSeries.distributionID != ubuntu_id).config(distinct=True)
+
+ def findNamedDistro(self, distro_name):
+ """Find the `Distribution` called `distro_name`."""
+ self.logger.debug("Finding distribution %s.", distro_name)
+ distro = getUtility(IDistributionSet).getByName(distro_name)
+ if distro is None:
raise LaunchpadScriptFailure(
"Distribution '%s' not found." % distro_name)
- return distribution
+ return distro
+
+ def findTargetDistros(self):
+ """Find the distribution(s) to process, based on arguments."""
+ if self.options.derived:
+ return self.findDerivedDistros()
+ else:
+ return [self.findNamedDistro(self.args[0])]
def validateArguments(self):
- if len(self.args) != 1:
- raise OptionValueError(
- "Need to be given exactly one non-option argument. "
- "Namely the distribution to process.")
-
+ """Validate command-line arguments."""
if self.options.ppa and self.options.copy_archives:
raise OptionValueError(
"Specify only one of copy archives or ppa archives.")
+ if self.options.derived:
+ if len(self.args) != 0:
+ raise OptionValueError(
+ "Can't combine --derived with a distribution name.")
+ else:
+ if len(self.args) != 1:
+ raise OptionValueError(
+ "Need to be given exactly one non-option argument. "
+ "Namely the distribution to process.")
def makeTargetPolicy(self):
"""Pick and instantiate a `TargetPolicy` based on given options."""
@@ -338,36 +365,44 @@
"Successfully processed queue item %d", queue_item.id)
return True
+ def processForDistro(self, distribution, target_policy):
+ """Process all queue items for a distribution.
+
+ Commits between items, except in dry-run mode.
+
+ :param distribution: The `Distribution` to process queue items for.
+ :param target_policy: The applicable `TargetPolicy`.
+ :return: A list of all successfully processed items' ids.
+ """
+ processed_queue_ids = []
+ for archive in target_policy.getTargetArchives(distribution):
+ description = target_policy.describeArchive(archive)
+ for distroseries in distribution.series:
+
+ self.logger.debug("Processing queue for %s %s" % (
+ distroseries.name, description))
+
+ queue_items = distroseries.getPackageUploads(
+ status=PackageUploadStatus.ACCEPTED, archive=archive)
+ for queue_item in queue_items:
+ if self.processQueueItem(queue_item):
+ processed_queue_ids.append(queue_item.id)
+ # Commit even on error; we may have altered the
+ # on-disk archive, so the partial state must
+ # make it to the DB.
+ self._commit()
+ return processed_queue_ids
+
def main(self):
"""Entry point for a LaunchpadScript."""
self.validateArguments()
target_policy = self.makeTargetPolicy()
- distribution = self.findTargetDistribution()
-
- processed_queue_ids = []
try:
- for archive in target_policy.getTargetArchives(distribution):
- description = target_policy.describeArchive(archive)
- for distroseries in distribution.series:
-
- self.logger.debug("Processing queue for %s %s" % (
- distroseries.name, description))
-
- queue_items = distroseries.getPackageUploads(
- status=PackageUploadStatus.ACCEPTED, archive=archive)
- for queue_item in queue_items:
- if self.processQueueItem(queue_item):
- processed_queue_ids.append(queue_item.id)
- # Commit even on error; we may have altered the
- # on-disk archive, so the partial state must
- # make it to the DB.
- self._commit()
-
- self._commit()
-
- target_policy.postprocessSuccesses(processed_queue_ids)
-
- self._commit()
+ for distro in self.findTargetDistros():
+ queue_ids = self.processForDistro(distro, target_policy)
+ self._commit()
+ target_policy.postprocessSuccesses(queue_ids)
+ self._commit()
finally:
self.logger.debug("Rolling back any remaining transactions.")
=== modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
--- lib/lp/soyuz/tests/test_processaccepted.py 2011-07-13 15:27:38 +0000
+++ lib/lp/soyuz/tests/test_processaccepted.py 2011-07-14 17:06:45 +0000
@@ -4,16 +4,20 @@
"""Test process-accepted.py"""
from cStringIO import StringIO
+from zope.component import getUtility
from canonical.launchpad.interfaces.lpstorm import IStore
from debian.deb822 import Changes
+from optparse import OptionValueError
from testtools.matchers import LessThan
from canonical.config import config
from canonical.launchpad.webapp.errorlog import ErrorReportingUtility
from canonical.testing.layers import LaunchpadZopelessLayer
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
from lp.registry.interfaces.series import SeriesStatus
from lp.services.log.logger import BufferLogger
+from lp.services.scripts.base import LaunchpadScriptFailure
from lp.soyuz.enums import (
ArchivePurpose,
PackagePublishingStatus,
@@ -189,6 +193,63 @@
self.assertEqual(
None, IStore(PackageUpload).get(PackageUpload, upload_id))
+ def test_validateArguments_requires_distro_by_default(self):
+ self.assertRaises(
+ OptionValueError, ProcessAccepted(test_args=[]).validateArguments)
+
+ def test_validateArguments_requires_no_distro_for_derived_run(self):
+ ProcessAccepted(test_args=['--derived']).validateArguments()
+ # The test is that this does not raise an exception.
+ pass
+
+ def test_validateArguments_does_not_accept_distro_for_derived_run(self):
+ distro = self.factory.makeDistribution()
+ script = ProcessAccepted(test_args=['--derived', distro.name])
+ self.assertRaises(OptionValueError, script.validateArguments)
+
+ def test_findTargetDistros_finds_named_distro(self):
+ distro = self.factory.makeDistribution()
+ script = ProcessAccepted(test_args=[distro.name])
+ self.assertContentEqual([distro], script.findTargetDistros())
+
+ def test_findNamedDistro_raises_error_if_not_found(self):
+ nonexistent_distro = self.factory.getUniqueString()
+ script = ProcessAccepted(test_args=[nonexistent_distro])
+ self.assertRaises(
+ LaunchpadScriptFailure,
+ script.findNamedDistro, nonexistent_distro)
+
+ def test_findTargetDistros_for_derived_finds_derived_distro(self):
+ dsp = self.factory.makeDistroSeriesParent()
+ script = ProcessAccepted(test_args=['--derived'])
+ self.assertIn(
+ dsp.derived_series.distribution, script.findDerivedDistros())
+
+ def test_findDerivedDistros_for_derived_ignores_non_derived_distros(self):
+ distro = self.factory.makeDistribution()
+ script = ProcessAccepted(test_args=['--derived'])
+ self.assertNotIn(distro, script.findDerivedDistros())
+
+ def test_findDerivedDistros_ignores_ubuntu_even_if_derived(self):
+ ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
+ self.factory.makeDistroSeriesParent(
+ derived_series=ubuntu.currentseries)
+ script = ProcessAccepted(test_args=['--derived'])
+ self.assertNotIn(ubuntu, script.findDerivedDistros())
+
+ def test_findDerivedDistros_finds_each_distro_just_once(self):
+ # Derived distros are not duplicated in the output of
+ # findDerivedDistros, even if they have multiple parents and
+ # multiple derived series.
+ dsp = self.factory.makeDistroSeriesParent()
+ distro = dsp.derived_series.distribution
+ other_series = self.factory.makeDistroSeries(distribution=distro)
+ self.factory.makeDistroSeriesParent(derived_series=other_series)
+ script = ProcessAccepted(test_args=['--derived'])
+ derived_distros = list(script.findDerivedDistros())
+ self.assertEqual(
+ 1, derived_distros.count(dsp.derived_series.distribution))
+
class TestBugsFromChangesFile(TestCaseWithFactory):
"""Test get_bugs_from_changes_file."""
