launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03064
lp:~allenap/launchpad/dd-initseries-bug-727105-derive-distro-series into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/dd-initseries-bug-727105-derive-distro-series into lp:launchpad with lp:~allenap/launchpad/dd-initseries-bug-727105-copy-options as a prerequisite.
Requested reviews:
Steve Kowalik (stevenk)
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #741916 in Launchpad itself: "+initseries: InitialiseDistroSeries should set parent"
https://bugs.launchpad.net/launchpad/+bug/741916
For more details, see:
https://code.launchpad.net/~allenap/launchpad/dd-initseries-bug-727105-derive-distro-series/+merge/54742
Currently, InitializeDistroSeries() needs the child series'
parent_series property set. However, other parts of the code consider
parent_series to be a marker for when a series has *already* been
initialized.
This branch changes the assumption. The InitialiseDistroSeriesJob now
takes another parameter, parent, that it stores in its metadata, and
InitialiseDistroSeries now also accepts a parent parameter. It is the
latter's responsibility for updating the child's parent_series
property. This also means that failed initializations (aka
derivations) don't leave the series in a limbo state.
IDistroSeries.parent_series is now editable by those with
launchpad.Admin. However, it is exported to the API as read-only, so
it cannot be modified except by internal code.
--
https://code.launchpad.net/~allenap/launchpad/dd-initseries-bug-727105-derive-distro-series/+merge/54742
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/dd-initseries-bug-727105-derive-distro-series into lp:launchpad.
=== modified file 'lib/lp/registry/configure.zcml'
--- lib/lp/registry/configure.zcml 2011-03-17 18:23:36 +0000
+++ lib/lp/registry/configure.zcml 2011-03-24 17:41:17 +0000
@@ -195,6 +195,9 @@
permission="launchpad.Edit"
interface="lp.registry.interfaces.distroseries.IDistroSeriesEditRestricted"/>
<require
+ permission="launchpad.Admin"
+ set_attributes="parent_series"/>
+ <require
permission="launchpad.TranslationsAdmin"
set_attributes="hide_all_translations defer_translation_imports"/>
<require
=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py 2011-03-23 12:21:08 +0000
+++ lib/lp/registry/model/distroseries.py 2011-03-24 17:41:17 +0000
@@ -1956,12 +1956,13 @@
child = distribution.newSeries(
name=name, displayname=displayname, title=title,
summary=summary, description=description,
- version=version, parent_series=self, owner=user)
+ version=version, parent_series=None, owner=user)
IStore(self).add(child)
else:
- if child.parent_series is not self:
+ if child.parent_series is not None:
raise DerivationError(
- "DistroSeries %s parent series isn't %s" % (
+ "DistroSeries %s parent series is %s, "
+ "but it must not be set" % (
child.name, self.name))
initialise_series = InitialiseDistroSeries(child)
try:
=== modified file 'lib/lp/registry/tests/test_derivedistroseries.py'
--- lib/lp/registry/tests/test_derivedistroseries.py 2011-03-09 09:53:59 +0000
+++ lib/lp/registry/tests/test_derivedistroseries.py 2011-03-24 17:41:17 +0000
@@ -6,6 +6,10 @@
__metaclass__ = type
+from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
+from zope.security.proxy import removeSecurityProxy
+
from canonical.testing.layers import LaunchpadFunctionalLayer
from lp.registry.interfaces.distroseries import DerivationError
from lp.soyuz.interfaces.distributionjob import (
@@ -17,8 +21,6 @@
TestCaseWithFactory,
)
from lp.testing.sampledata import ADMIN_EMAIL
-from zope.component import getUtility
-from zope.security.interfaces import Unauthorized
class TestDeriveDistroSeries(TestCaseWithFactory):
@@ -29,8 +31,7 @@
super(TestDeriveDistroSeries, self).setUp()
self.soyuz = self.factory.makeTeam(name='soyuz-team')
self.parent = self.factory.makeDistroSeries()
- self.child = self.factory.makeDistroSeries(
- parent_series=self.parent)
+ self.child = self.factory.makeDistroSeries()
def test_no_permission_to_call(self):
login(ADMIN_EMAIL)
@@ -50,13 +51,15 @@
self.parent.deriveDistroSeries, self.soyuz.teamowner,
'newdistro')
- def test_parent_is_not_self(self):
- other = self.factory.makeDistroSeries()
+ def test_parent_is_not_set(self):
+ # When parent_series is set it means that the distroseries has already
+ # been derived, and it is forbidden to derive more than once.
+ removeSecurityProxy(self.child).parent_series = self.parent
self.assertRaisesWithContent(
DerivationError,
- "DistroSeries %s parent series isn't %s" % (
- self.child.name, other.name),
- other.deriveDistroSeries, self.soyuz.teamowner,
+ ("DistroSeries {self.child.name} parent series is "
+ "{self.parent.name}, but it must not be set").format(self=self),
+ self.parent.deriveDistroSeries, self.soyuz.teamowner,
self.child.name, self.child.distribution)
def test_create_new_distroseries(self):
@@ -79,8 +82,7 @@
# Make two distroseries with the same name and different
# distributions to check that the right distroseries is initialised.
self.factory.makeDistroSeries(name='bar')
- bar = self.factory.makeDistroSeries(
- name='bar', parent_series=self.parent)
+ bar = self.factory.makeDistroSeries(name='bar')
self.parent.deriveDistroSeries(
self.soyuz.teamowner, 'bar', distribution=bar.parent,
displayname='Bar', title='The Bar', summary='Bar',
=== modified file 'lib/lp/soyuz/model/distributionjob.py'
--- lib/lp/soyuz/model/distributionjob.py 2011-03-10 14:05:51 +0000
+++ lib/lp/soyuz/model/distributionjob.py 2011-03-24 17:41:17 +0000
@@ -8,27 +8,28 @@
"DistributionJobDerived",
]
+from lazr.delegates import delegates
import simplejson
-
-from storm.locals import And, Int, Reference, Unicode
-
+from storm.locals import (
+ And,
+ Int,
+ Reference,
+ Unicode,
+ )
from zope.interface import implements
from canonical.database.enumcol import EnumCol
from canonical.launchpad.interfaces.lpstorm import IStore
-
-from lazr.delegates import delegates
-
from lp.app.errors import NotFoundError
from lp.registry.model.distribution import Distribution
from lp.registry.model.distroseries import DistroSeries
+from lp.services.database.stormbase import StormBase
+from lp.services.job.model.job import Job
+from lp.services.job.runner import BaseRunnableJob
from lp.soyuz.interfaces.distributionjob import (
DistributionJobType,
IDistributionJob,
)
-from lp.services.job.model.job import Job
-from lp.services.job.runner import BaseRunnableJob
-from lp.services.database.stormbase import StormBase
class DistributionJob(StormBase):
@@ -106,7 +107,7 @@
def getOopsVars(self):
"""See `IRunnableJob`."""
- vars = BaseRunnableJob.getOopsVars(self)
+ vars = super(DistributionJobDerived, self).getOopsVars()
vars.extend([
('distribution_id', self.context.distribution.id),
('distroseries_id', self.context.distroseries.id),
=== modified file 'lib/lp/soyuz/model/initialisedistroseriesjob.py'
--- lib/lp/soyuz/model/initialisedistroseriesjob.py 2010-10-13 05:04:41 +0000
+++ lib/lp/soyuz/model/initialisedistroseriesjob.py 2011-03-24 17:41:17 +0000
@@ -7,10 +7,16 @@
"InitialiseDistroSeriesJob",
]
-from zope.interface import classProvides, implements
-
-from canonical.launchpad.interfaces.lpstorm import IMasterStore
-
+from zope.interface import (
+ classProvides,
+ implements,
+ )
+
+from canonical.launchpad.interfaces.lpstorm import (
+ IMasterStore,
+ IStore,
+ )
+from lp.registry.model.distroseries import DistroSeries
from lp.soyuz.interfaces.distributionjob import (
DistributionJobType,
IInitialiseDistroSeriesJob,
@@ -20,9 +26,7 @@
DistributionJob,
DistributionJobDerived,
)
-from lp.soyuz.scripts.initialise_distroseries import (
- InitialiseDistroSeries,
- )
+from lp.soyuz.scripts.initialise_distroseries import InitialiseDistroSeries
class InitialiseDistroSeriesJob(DistributionJobDerived):
@@ -33,21 +37,26 @@
classProvides(IInitialiseDistroSeriesJobSource)
@classmethod
- def create(cls, distroseries, arches=(), packagesets=(),
- rebuild=False):
+ def create(cls, parent, child, arches=(), packagesets=(), rebuild=False):
"""See `IInitialiseDistroSeriesJob`."""
metadata = {
+ 'parent': parent.id,
'arches': arches,
'packagesets': packagesets,
'rebuild': rebuild,
}
job = DistributionJob(
- distroseries.distribution, distroseries, cls.class_job_type,
+ child.distribution, child, cls.class_job_type,
metadata)
IMasterStore(DistributionJob).add(job)
return cls(job)
@property
+ def parent(self):
+ return IStore(DistroSeries).get(
+ DistroSeries, self.metadata["parent"])
+
+ @property
def arches(self):
return tuple(self.metadata['arches'])
@@ -62,7 +71,13 @@
def run(self):
"""See `IRunnableJob`."""
ids = InitialiseDistroSeries(
- self.distroseries, self.arches, self.packagesets,
- self.rebuild)
+ self.parent, self.distroseries, self.arches,
+ self.packagesets, self.rebuild)
ids.check()
ids.initialise()
+
+ def getOopsVars(self):
+ """See `IRunnableJob`."""
+ vars = super(InitialiseDistroSeriesJob, self).getOopsVars()
+ vars.append(('parent_distroseries_id', self.metadata.get("parent")))
+ return vars
=== modified file 'lib/lp/soyuz/scripts/initialise_distroseries.py'
--- lib/lp/soyuz/scripts/initialise_distroseries.py 2011-01-15 06:32:40 +0000
+++ lib/lp/soyuz/scripts/initialise_distroseries.py 2011-03-24 17:41:17 +0000
@@ -40,7 +40,8 @@
Preconditions:
The distroseries must exist, and be completly unused, with no source
or binary packages existing, as well as no distroarchseries set up.
- Section and component selections must be empty.
+ Section and component selections must be empty. It must not have a
+ parent series.
Outcome:
The distroarchseries set up in the parent series will be copied.
@@ -60,11 +61,11 @@
"""
def __init__(
- self, distroseries, arches=(), packagesets=(), rebuild=False):
+ self, parent, distroseries, arches=(), packagesets=(), rebuild=False):
# Avoid circular imports
from lp.registry.model.distroseries import DistroSeries
+ self.parent = parent
self.distroseries = distroseries
- self.parent = self.distroseries.parent_series
self.arches = arches
self.packagesets = [
ensure_unicode(packageset) for packageset in packagesets]
@@ -72,8 +73,9 @@
self._store = IMasterStore(DistroSeries)
def check(self):
- if self.parent is None:
- raise InitialisationError("Parent series required.")
+ if self.distroseries.parent_series is not None:
+ raise InitialisationError(
+ "Parent series has already been initialized.")
self._checkBuilds()
self._checkQueue()
self._checkSeries()
@@ -127,10 +129,14 @@
raise InitialisationError(error)
def initialise(self):
+ self._set_parent()
self._copy_architectures()
self._copy_packages()
self._copy_packagesets()
+ def _set_parent(self):
+ self.distroseries.parent_series = self.parent
+
def _copy_architectures(self):
include = ''
if self.arches:
=== modified file 'lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py 2010-10-14 12:56:31 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py 2011-03-24 17:41:17 +0000
@@ -9,6 +9,8 @@
import subprocess
import sys
+from testtools.content import Content
+from testtools.content_type import UTF8_TEXT
import transaction
from zope.component import getUtility
@@ -83,17 +85,11 @@
pocket=PackagePublishingPocket.RELEASE,
status=PackagePublishingStatus.PUBLISHED)
- def test_failure_with_no_parent_series(self):
- # Initialising a new distro series requires a parent series to be set
- ids = InitialiseDistroSeries(self.factory.makeDistroSeries())
- self.assertRaisesWithContent(
- InitialisationError, "Parent series required.", ids.check)
-
def test_failure_for_already_released_distroseries(self):
# Initialising a distro series that has already been used will error
- child = self.factory.makeDistroSeries(parent_series=self.parent)
+ child = self.factory.makeDistroSeries()
self.factory.makeDistroArchSeries(distroseries=child)
- ids = InitialiseDistroSeries(child)
+ ids = InitialiseDistroSeries(self.parent, child)
self.assertRaisesWithContent(
InitialisationError,
"Can not copy distroarchseries from parent, there are already "
@@ -105,9 +101,8 @@
distroseries=self.parent,
pocket=PackagePublishingPocket.RELEASE)
source.createMissingBuilds()
- child = self.factory.makeDistroSeries(
- parent_series=self.parent)
- ids = InitialiseDistroSeries(child)
+ child = self.factory.makeDistroSeries()
+ ids = InitialiseDistroSeries(self.parent, child)
self.assertRaisesWithContent(
InitialisationError, "Parent series has pending builds.",
ids.check)
@@ -118,8 +113,8 @@
self.parent.createQueueEntry(
PackagePublishingPocket.RELEASE,
'foo.changes', 'bar', self.parent.main_archive)
- child = self.factory.makeDistroSeries(parent_series=self.parent)
- ids = InitialiseDistroSeries(child)
+ child = self.factory.makeDistroSeries()
+ ids = InitialiseDistroSeries(self.parent, child)
self.assertRaisesWithContent(
InitialisationError, "Parent series queues are not empty.",
ids.check)
@@ -159,8 +154,9 @@
SourcePackageFormat.FORMAT_1_0))
def _full_initialise(self, arches=(), packagesets=(), rebuild=False):
- child = self.factory.makeDistroSeries(parent_series=self.parent)
- ids = InitialiseDistroSeries(child, arches, packagesets, rebuild)
+ child = self.factory.makeDistroSeries()
+ ids = InitialiseDistroSeries(
+ self.parent, child, arches, packagesets, rebuild)
ids.check()
ids.initialise()
return child
@@ -322,6 +318,11 @@
test1.addSources('udev')
getUtility(IArchivePermissionSet).newPackagesetUploader(
self.parent.main_archive, uploader, test1)
+ # The child must have a parent series because initialise-from-parent
+ # expects it; this script supports the old-style derivation of
+ # distribution series where the parent series is specified at the time
+ # of adding the series. New-style derivation leaves the specification
+ # of the parent series until later.
child = self.factory.makeDistroSeries(parent_series=self.parent)
transaction.commit()
ifp = os.path.join(
@@ -331,6 +332,8 @@
[sys.executable, ifp, "-vv", "-d", child.parent.name,
child.name], stdout=subprocess.PIPE, stderr=subprocess.PIPE)
stdout, stderr = process.communicate()
+ self.addDetail("stdout", Content(UTF8_TEXT, lambda: stdout))
+ self.addDetail("stderr", Content(UTF8_TEXT, lambda: stderr))
self.assertEqual(process.returncode, 0)
self.assertTrue(
"DEBUG Committing transaction." in stderr.split('\n'))
=== modified file 'lib/lp/soyuz/tests/test_initialisedistroseriesjob.py'
--- lib/lp/soyuz/tests/test_initialisedistroseriesjob.py 2010-12-02 16:13:51 +0000
+++ lib/lp/soyuz/tests/test_initialisedistroseriesjob.py 2011-03-24 17:41:17 +0000
@@ -13,7 +13,11 @@
from zope.security.proxy import removeSecurityProxy
from canonical.config import config
-from canonical.testing import LaunchpadZopelessLayer
+from canonical.database.sqlbase import flush_database_caches
+from canonical.testing import (
+ DatabaseFunctionalLayer,
+ LaunchpadZopelessLayer,
+ )
from lp.buildmaster.enums import BuildStatus
from lp.registry.interfaces.pocket import PackagePublishingPocket
from lp.soyuz.interfaces.distributionjob import (
@@ -30,18 +34,23 @@
class InitialiseDistroSeriesJobTests(TestCaseWithFactory):
"""Test case for InitialiseDistroSeriesJob."""
- layer = LaunchpadZopelessLayer
+ layer = DatabaseFunctionalLayer
+
+ @property
+ def job_source(self):
+ return getUtility(IInitialiseDistroSeriesJobSource)
def test_getOopsVars(self):
+ parent = self.factory.makeDistroSeries()
distroseries = self.factory.makeDistroSeries()
- job = getUtility(IInitialiseDistroSeriesJobSource).create(
- distroseries)
+ job = self.job_source.create(parent, distroseries)
vars = job.getOopsVars()
naked_job = removeSecurityProxy(job)
self.assertIn(
('distribution_id', distroseries.distribution.id), vars)
self.assertIn(('distroseries_id', distroseries.id), vars)
self.assertIn(('distribution_job_id', naked_job.context.id), vars)
+ self.assertIn(('parent_distroseries_id', parent.id), vars)
def _getJobs(self):
"""Return the pending InitialiseDistroSeriesJobs as a list."""
@@ -53,52 +62,60 @@
return len(self._getJobs())
def test_create_only_creates_one(self):
+ parent = self.factory.makeDistroSeries()
distroseries = self.factory.makeDistroSeries()
# If there's already a InitialiseDistroSeriesJob for a
# DistroSeries, InitialiseDistroSeriesJob.create() won't create
# a new one.
- getUtility(IInitialiseDistroSeriesJobSource).create(
- distroseries)
- transaction.commit()
-
- # There will now be one job in the queue.
- self.assertEqual(1, self._getJobCount())
-
- getUtility(IInitialiseDistroSeriesJobSource).create(
- distroseries)
-
- # This is less than ideal
- self.assertRaises(IntegrityError, self._getJobCount)
-
- def test_run(self):
- """Test that InitialiseDistroSeriesJob.run() actually
- initialises builds and copies from the parent."""
- distroseries = self.factory.makeDistroSeries()
-
- job = getUtility(IInitialiseDistroSeriesJobSource).create(
- distroseries)
-
- # Since our new distroseries doesn't have a parent set, and the first
- # thing that run() will execute is checking the distroseries, if it
- # returns an InitialisationError, then it's good.
+ self.job_source.create(parent, distroseries)
+ self.job_source.create(parent, distroseries)
+ self.assertRaises(IntegrityError, flush_database_caches)
+
+ def test_run_with_parent_series_already_set(self):
+ # InitialisationError is raised if the parent series is already set on
+ # the child.
+ parent = self.factory.makeDistroSeries()
+ distroseries = self.factory.makeDistroSeries(parent_series=parent)
+ job = self.job_source.create(parent, distroseries)
self.assertRaisesWithContent(
- InitialisationError, "Parent series required.", job.run)
+ InitialisationError,
+ "Parent series has already been initialized.",
+ job.run)
def test_arguments(self):
"""Test that InitialiseDistroSeriesJob specified with arguments can
be gotten out again."""
+ parent = self.factory.makeDistroSeries()
distroseries = self.factory.makeDistroSeries()
arches = (u'i386', u'amd64')
packagesets = (u'foo', u'bar', u'baz')
- job = getUtility(IInitialiseDistroSeriesJobSource).create(
- distroseries, arches, packagesets)
+ job = self.job_source.create(
+ parent, distroseries, arches, packagesets)
naked_job = removeSecurityProxy(job)
self.assertEqual(naked_job.distroseries, distroseries)
self.assertEqual(naked_job.arches, arches)
self.assertEqual(naked_job.packagesets, packagesets)
self.assertEqual(naked_job.rebuild, False)
+ self.assertEqual(naked_job.metadata["parent"], parent.id)
+
+ def test_parent(self):
+ parent = self.factory.makeDistroSeries()
+ distroseries = self.factory.makeDistroSeries()
+ job = self.job_source.create(parent, distroseries)
+ naked_job = removeSecurityProxy(job)
+ self.assertEqual(parent, naked_job.parent)
+
+
+class InitialiseDistroSeriesJobTestsWithPackages(TestCaseWithFactory):
+ """Test case for InitialiseDistroSeriesJob."""
+
+ layer = LaunchpadZopelessLayer
+
+ @property
+ def job_source(self):
+ return getUtility(IInitialiseDistroSeriesJobSource)
def _create_child(self):
pf = self.factory.makeProcessorFamily()
@@ -125,16 +142,14 @@
distroseries=parent)
test1.addSources('udev')
parent.updatePackageCount()
- child = self.factory.makeDistroSeries(parent_series=parent)
-
- # Make sure everything hits the database, switching db users
- # aborts.
+ child = self.factory.makeDistroSeries()
+ # Make sure everything hits the database, switching db users aborts.
transaction.commit()
return parent, child
def test_job(self):
parent, child = self._create_child()
- job = getUtility(IInitialiseDistroSeriesJobSource).create(child)
+ job = self.job_source.create(parent, child)
self.layer.switchDbUser('initialisedistroseries')
job.run()
@@ -145,8 +160,9 @@
def test_job_with_arguments(self):
parent, child = self._create_child()
arch = parent.nominatedarchindep.architecturetag
- job = getUtility(IInitialiseDistroSeriesJobSource).create(
- child, packagesets=('test1',), arches=(arch,), rebuild=True)
+ job = self.job_source.create(
+ parent, child, packagesets=('test1',), arches=(arch,),
+ rebuild=True)
self.layer.switchDbUser('initialisedistroseries')
job.run()
=== modified file 'scripts/ftpmaster-tools/initialise-from-parent.py'
--- scripts/ftpmaster-tools/initialise-from-parent.py 2010-11-08 12:52:43 +0000
+++ scripts/ftpmaster-tools/initialise-from-parent.py 2011-03-24 17:41:17 +0000
@@ -5,23 +5,27 @@
"""Initialise a new distroseries from its parent series."""
+from optparse import OptionParser
+import sys
+
import _pythonpath
-
-import sys
-from optparse import OptionParser
-
+from contrib.glock import GlobalLock
from zope.component import getUtility
-from contrib.glock import GlobalLock
from canonical.config import config
-from lp.registry.interfaces.distribution import IDistributionSet
from canonical.launchpad.scripts import (
- execute_zcml_for_scripts, logger, logger_options)
+ execute_zcml_for_scripts,
+ logger,
+ logger_options,
+ )
from canonical.lp import initZopeless
-
from lp.app.errors import NotFoundError
+from lp.registry.interfaces.distribution import IDistributionSet
from lp.soyuz.scripts.initialise_distroseries import (
- InitialisationError, InitialiseDistroSeries)
+ InitialisationError,
+ InitialiseDistroSeries,
+ )
+
def main():
# Parse command-line arguments
@@ -77,7 +81,12 @@
arches = ()
if options.arches is not None:
arches = tuple(options.arches.split(','))
- ids = InitialiseDistroSeries(distroseries, arches)
+ # InitialiseDistroSeries does not like it if the parent series is
+ # specified on the child, so we must unset it and pass it in. This is
+ # a temporary hack until confidence in InitialiseDistroSeriesJob is
+ # good, at which point this script will be obsolete.
+ parent, distroseries.parent_series = distroseries.parent_series, None
+ ids = InitialiseDistroSeries(parent, distroseries, arches)
ids.check()
log.debug('initialising from parent, copying publishing records.')
ids.initialise()
@@ -99,4 +108,3 @@
if __name__ == '__main__':
sys.exit(main())
-
Follow ups
