← Back to team overview

launchpad-reviewers team mailing list archive

[Merge] lp:~stevenk/launchpad/db-add-derivedistroseries-api into lp:launchpad

 

Steve Kowalik has proposed merging lp:~stevenk/launchpad/db-add-derivedistroseries-api into lp:launchpad with lp:~stevenk/launchpad/db-add-parameters-to-idsjob as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #644196 Support deriving a distroseries via the API
  https://bugs.launchpad.net/bugs/644196


This branch adds a new method to IDistroSeries: deriveDistroSeries().

This method is intended to be called via API scripts for both the distro use-case as well as the Linaro use-case to initialise a new distroseries (which may or may not exist), from an existing distroseries. It will create the new distroseries if it doesn't exist, under the specified distribution, unless it isn't specified, in which case it will use the current distroseries's distribution (.parent in the interface).

After it has verified everything, and created the distroseries if need be, it checks the details submitted via the InitialiseDistroSeries.check() method, and if that doesn't raise an InitislationError exception, an InitialiseDistroSeriesJob is created. The method doesn't return the job id, since there is no way for the user to check the status of the job.

I have written two seperate tests, one that calls the method directly and tests it, and a lighter doctest that checks via the API.

I have discussed this change with both Julian and Michael, and they agreed that it sounded good.

To test: bin/test -vvt test_derivedistroseries -t xx-derivedistroseries.txt

This branch also drive-bys two mis-spellings of distribution, since I noticed them.

This MP supersedes the one linked at https://code.edge.launchpad.net/~stevenk/launchpad/db-add-derivedistroseries-api/+merge/35500, which was approved by Gavin.
-- 
https://code.launchpad.net/~stevenk/launchpad/db-add-derivedistroseries-api/+merge/38189
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/db-add-derivedistroseries-api into lp:launchpad.
=== modified file 'lib/lp/registry/interfaces/distroseries.py'
--- lib/lp/registry/interfaces/distroseries.py	2010-09-28 18:11:41 +0000
+++ lib/lp/registry/interfaces/distroseries.py	2010-10-12 08:37:57 +0000
@@ -8,6 +8,7 @@
 __metaclass__ = type
 
 __all__ = [
+    'DerivationError',
     'IDistroSeries',
     'IDistroSeriesEditRestricted',
     'IDistroSeriesPublic',
@@ -16,20 +17,25 @@
 
 from lazr.enum import DBEnumeratedType
 from lazr.restful.declarations import (
+    call_with,
     export_as_webservice_entry,
     export_factory_operation,
     export_read_operation,
+    export_write_operation,
     exported,
     LAZR_WEBSERVICE_EXPORTED,
     operation_parameters,
     operation_returns_collection_of,
     operation_returns_entry,
     rename_parameters_as,
+    REQUEST_USER,
+    webservice_error,
     )
 from lazr.restful.fields import (
     Reference,
     ReferenceChoice,
     )
+from lazr.restful.interface import copy_field
 from zope.component import getUtility
 from zope.interface import (
     Attribute,
@@ -39,6 +45,7 @@
     Bool,
     Choice,
     Datetime,
+    List,
     Object,
     TextLine,
     )
@@ -673,8 +680,8 @@
         If sourcename is passed, only packages that are built from
         source packages by that name will be returned.
         If archive is passed, restricted the results to the given archive,
-        if it is suppressed the results will be restricted to the distribtion
-        'main_archive'.
+        if it is suppressed the results will be restricted to the
+        distribution 'main_archive'.
         """
 
     def getSourcePackagePublishing(status, pocket, component=None,
@@ -683,8 +690,8 @@
 
         According status and pocket.
         If archive is passed, restricted the results to the given archive,
-        if it is suppressed the results will be restricted to the distribtion
-        'main_archive'.
+        if it is suppressed the results will be restricted to the
+        distribution 'main_archive'.
         """
 
     def getBinaryPackageCaches(archive=None):
@@ -789,6 +796,69 @@
         :param format: The SourcePackageFormat to check.
         """
 
+    @operation_parameters(
+        name=copy_field(name, required=True),
+        displayname=copy_field(displayname, required=False),
+        title=copy_field(title, required=False),
+        summary=TextLine(
+            title=_("The summary of the distroseries to derive."),
+            required=False),
+        description=copy_field(description, required=False),
+        version=copy_field(version, required=False),
+        distribution=copy_field(distribution, required=False),
+        status=copy_field(status, required=False),
+        architectures=List(
+            title=_("The list of architectures to copy to the derived "
+            "distroseries."),
+            required=False),
+        packagesets=List(
+            title=_("The list of packagesets to copy to the derived "
+            "distroseries"),
+            required=False),
+        rebuild=Bool(
+            title=_("If binaries will be copied to the derived "
+            "distroseries."),
+            required=True),
+        )
+    @call_with(user=REQUEST_USER)
+    @export_write_operation()
+    def deriveDistroSeries(
+        user, name, displayname, title, summary, description, version,
+        distribution, status, architectures, packagesets, rebuild):
+        """Derive a distroseries from this one.
+
+        This method performs checks, can create the new distroseries if
+        necessary, and then creates a job to populate the new
+        distroseries.
+
+        :param name: The name of the new distroseries we will create if it
+            doesn't exist, or the name of the distroseries we will look
+            up, and then initialise.
+        :param displayname: The Display Name for the new distroseries.
+            If the distroseries already exists this parameter is ignored.
+        :param title: The Title for the new distroseries. If the
+            distroseries already exists this parameter is ignored.
+        :param summary: The Summary for the new distroseries. If the
+            distroseries already exists this parameter is ignored.
+        :param description: The Description for the new distroseries. If the
+            distroseries already exists this parameter is ignored.
+        :param version: The version for the new distroseries. If the
+            distroseries already exists this parameter is ignored.
+        :param distribution: The distribution the derived series will
+            belong to. If it isn't specified this distroseries'
+            distribution is used.
+        :param status: The status the new distroseries will be created
+            in. If the distroseries isn't specified, this parameter will
+            be ignored. Defaults to FROZEN.
+        :param architectures: The architectures to copy to the derived
+            series. If not specified, all of the architectures are copied.
+        :param packagesets: The packagesets to copy to the derived series.
+            If not specified, all of the packagesets are copied.
+        :param rebuild: Whether binaries will be copied to the derived
+            series. If it's true, they will not be, and if it's false, they
+            will be.
+        """
+
 
 class IDistroSeries(IDistroSeriesEditRestricted, IDistroSeriesPublic,
                     IStructuralSubscriptionTarget):
@@ -856,5 +926,11 @@
         """
 
 
+class DerivationError(Exception):
+    """Raised when there is a problem deriving a distroseries."""
+    webservice_error(400) # Bad Request
+    _message_prefix = "Error deriving distro series"
+
+
 # Monkey patch for circular import avoidance done in
 # _schema_circular_imports.py

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2010-10-03 15:30:06 +0000
+++ lib/lp/registry/model/distroseries.py	2010-10-12 08:37:57 +0000
@@ -35,6 +35,7 @@
     )
 from zope.component import getUtility
 from zope.interface import implements
+from zope.security.interfaces import Unauthorized
 
 from canonical.database.constants import (
     DEFAULT,
@@ -88,6 +89,7 @@
     )
 from lp.bugs.model.bugtask import BugTask
 from lp.registry.interfaces.distroseries import (
+    DerivationError,
     IDistroSeries,
     IDistroSeriesSet,
     )
@@ -128,6 +130,9 @@
 from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet
 from lp.soyuz.interfaces.binarypackagename import IBinaryPackageName
 from lp.soyuz.interfaces.buildrecords import IHasBuildRecords
+from lp.soyuz.interfaces.distributionjob import (
+    IInitialiseDistroSeriesJobSource,
+    )
 from lp.soyuz.interfaces.publishing import (
     active_publishing_status,
     ICanPublishPackages,
@@ -162,6 +167,10 @@
     )
 from lp.soyuz.model.section import Section
 from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
+from lp.soyuz.scripts.initialise_distroseries import (
+    InitialisationError,
+    InitialiseDistroSeries,
+    )
 from lp.translations.interfaces.languagepack import LanguagePackType
 from lp.translations.model.distroseries_translations_copy import (
     copy_active_translations,
@@ -1847,6 +1856,47 @@
             ISourcePackageFormatSelectionSet).getBySeriesAndFormat(
                 self, format) is not None
 
+    def deriveDistroSeries(
+        self, user, name, distribution=None, displayname=None,
+        title=None, summary=None, description=None, version=None,
+        status=SeriesStatus.FROZEN, architectures=(), packagesets=(),
+        rebuild=False):
+        """See `IDistroSeries`."""
+        # XXX StevenK bug=643369 This should be in the security adapter
+        # This should be allowed if the user is a driver for self.parent
+        # or the child.parent's drivers.
+        if not (user.inTeam('soyuz-team') or user.inTeam('admins')):
+            raise Unauthorized
+        child = IStore(self).find(DistroSeries, name=name).one()
+        if child is None:
+            if distribution is None:
+                distribution = self.distribution
+            for param in (
+                displayname, title, summary, description, version):
+                if param is None or len(param) == 0:
+                    raise DerivationError(
+                        "Display Name, Title, Summary, Description and"
+                        " Version all need to be set when creating a"
+                         " distroseries")
+            child = distribution.newSeries(
+                name=name, displayname=displayname, title=title,
+                summary=summary, description=description,
+                version=version, parent_series=self, owner=user)
+            child.status = status
+            IStore(self).add(child)
+        else:
+            if child.parent_series is not self:
+                raise DerivationError(
+                    "DistroSeries %s parent series isn't %s" % (
+                        child.name, self.name))
+        initialise_series = InitialiseDistroSeries(child)
+        try:
+            initialise_series.check()
+        except InitialisationError, e:
+            raise DerivationError(e)
+        getUtility(IInitialiseDistroSeriesJobSource).create(
+            child, architectures, packagesets, rebuild)
+
 
 class DistroSeriesSet:
     implements(IDistroSeriesSet)

=== added file 'lib/lp/registry/stories/webservice/xx-derivedistroseries.txt'
--- lib/lp/registry/stories/webservice/xx-derivedistroseries.txt	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/stories/webservice/xx-derivedistroseries.txt	2010-10-12 08:37:57 +0000
@@ -0,0 +1,68 @@
+Derive Distributions
+--------------------
+
+Using the DistroSeries.deriveDistroSeries() function, we can call it with the
+parent distroseries. We can call it with the distroseries already created,
+or it can create it for the user.
+
+Set Up
+------
+
+    >>> login('admin@xxxxxxxxxxxxx')
+    >>> soyuz = factory.makeTeam(name='soyuz-team')
+    >>> parent = factory.makeDistroSeries()
+    >>> child = factory.makeDistroSeries(parent_series=parent)
+    >>> other = factory.makeDistroSeries()
+    >>> logout()
+    >>> from canonical.launchpad.testing.pages import webservice_for_person
+    >>> from canonical.launchpad.webapp.interfaces import OAuthPermission
+    >>> soyuz_webservice = webservice_for_person(
+    ...     soyuz.teamowner, permission=OAuthPermission.WRITE_PUBLIC)
+
+Calling
+-------
+
+We can't call .deriveDistroSeries() with a distroseries that isn't the
+child's parent
+
+    >>> series_url = '/%s/%s' % (parent.parent.name, parent.name)
+    >>> other_series_url = '/%s/%s' % (
+    ...    other.parent.name, other.name)
+    >>> child_name = child.name
+    >>> series = webservice.get(series_url).jsonBody()
+    >>> other_series = webservice.get(other_series_url).jsonBody()
+    >>> derived = soyuz_webservice.named_post(
+    ...     other_series['self_link'], 'deriveDistroSeries', {},
+    ...     name=child_name, rebuild=False)
+    >>> print derived
+    HTTP/1.1 400 Bad Request
+    Status: 400 Bad Request
+    ...
+    <BLANKLINE>
+    DistroSeries ... parent series isn't ...
+    <BLANKLINE>
+    ...
+
+If we call it correctly, it works.
+
+    >>> derived = soyuz_webservice.named_post(
+    ...     series['self_link'], 'deriveDistroSeries', {},
+    ...     name=child_name, rebuild=False)
+    >>> print derived
+    HTTP/1.1 200 Ok
+    Status: 200 Ok
+    ...
+    <BLANKLINE>
+    ...
+
+And we can verify the job exists.
+
+    >>> from zope.component import getUtility
+    >>> from lp.soyuz.interfaces.distributionjob import (
+    ...     IInitialiseDistroSeriesJobSource)
+    >>> login('admin@xxxxxxxxxxxxx')
+    >>> [job] = list(
+    ...     getUtility(IInitialiseDistroSeriesJobSource).iterReady())
+    >>> job.distroseries == child
+    True
+

=== added file 'lib/lp/registry/tests/test_derivedistroseries.py'
--- lib/lp/registry/tests/test_derivedistroseries.py	1970-01-01 00:00:00 +0000
+++ lib/lp/registry/tests/test_derivedistroseries.py	2010-10-12 08:37:57 +0000
@@ -0,0 +1,78 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test initialising a distroseries using
+IDistroSeries.deriveDistroSeries."""
+
+__metaclass__ = type
+
+from canonical.testing.layers import LaunchpadFunctionalLayer
+from lp.registry.interfaces.distroseries import DerivationError
+from lp.soyuz.interfaces.distributionjob import (
+    IInitialiseDistroSeriesJobSource,
+    )
+from lp.testing import (
+    login,
+    logout,
+    TestCaseWithFactory,
+    )
+from zope.component import getUtility
+from zope.security.interfaces import Unauthorized
+
+
+class TestDeriveDistroSeries(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def setUp(self):
+        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)
+
+    def test_no_permission_to_call(self):
+        login('admin@xxxxxxxxxxxxx')
+        person = self.factory.makePerson()
+        logout()
+        self.assertRaises(
+            Unauthorized, self.parent.deriveDistroSeries, person,
+            self.child.name)
+
+    def test_no_distroseries_and_no_arguments(self):
+        """Test that calling deriveDistroSeries() when the distroseries
+        doesn't exist, and not enough arguments are specified that the
+        function errors."""
+        self.assertRaisesWithContent(
+            DerivationError,
+            'Display Name, Title, Summary, Description and Version all '
+            'need to be set when creating a distroseries',
+            self.parent.deriveDistroSeries, self.soyuz.teamowner,
+            'foobuntu')
+
+    def test_parent_is_not_self(self):
+        login('admin@xxxxxxxxxxxxx')
+        other = self.factory.makeDistroSeries()
+        logout()
+        self.assertRaisesWithContent(
+            DerivationError,
+            "DistroSeries %s parent series isn't %s" % (
+                self.child.name, other.name),
+            other.deriveDistroSeries, self.soyuz.teamowner,
+            self.child.name)
+
+    def test_create_new_distroseries(self):
+        self.parent.deriveDistroSeries(
+            self.soyuz.teamowner, self.child.name)
+        [job] = list(
+            getUtility(IInitialiseDistroSeriesJobSource).iterReady())
+        self.assertEqual(job.distroseries, self.child)
+
+    def test_create_fully_new_distroseries(self):
+        self.parent.deriveDistroSeries(
+            self.soyuz.teamowner, 'foobuntu', displayname='Foobuntu',
+            title='The Foobuntu', summary='Foobuntu',
+            description='Foobuntu is great', version='11.11')
+        [job] = list(
+            getUtility(IInitialiseDistroSeriesJobSource).iterReady())
+        self.assertEqual(job.distroseries.name, 'foobuntu')

=== modified file 'lib/lp/soyuz/configure.zcml'
--- lib/lp/soyuz/configure.zcml	2010-10-06 18:53:53 +0000
+++ lib/lp/soyuz/configure.zcml	2010-10-12 08:37:57 +0000
@@ -905,9 +905,12 @@
       provides="lp.soyuz.interfaces.distributionjob.IInitialiseDistroSeriesJobSource">
         <allow interface="lp.soyuz.interfaces.distributionjob.IInitialiseDistroSeriesJobSource"/>
     </securedutility>
-
+    <class class="lp.soyuz.model.distributionjob.DistributionJob">
+        <allow interface="lp.soyuz.interfaces.distributionjob.IDistributionJob" />
+    </class>
     <class class="lp.soyuz.model.initialisedistroseriesjob.InitialiseDistroSeriesJob">
-        <allow interface="lp.services.job.interfaces.job.IRunnableJob" />
+        <allow interface="lp.soyuz.interfaces.distributionjob.IInitialiseDistroSeriesJob" />
+        <allow interface="lp.soyuz.interfaces.distributionjob.IDistributionJob" />
     </class>
 
 </configure>

=== modified file 'lib/lp/soyuz/scripts/initialise_distroseries.py'
--- lib/lp/soyuz/scripts/initialise_distroseries.py	2010-10-12 08:37:55 +0000
+++ lib/lp/soyuz/scripts/initialise_distroseries.py	2010-10-12 08:37:57 +0000
@@ -16,7 +16,6 @@
 from canonical.launchpad.interfaces.lpstorm import IMasterStore
 from lp.buildmaster.enums import BuildStatus
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.registry.model.distroseries import DistroSeries
 from lp.soyuz.adapters.packagelocation import PackageLocation
 from lp.soyuz.enums import (
     ArchivePurpose,
@@ -61,7 +60,8 @@
 
     def __init__(
         self, distroseries, arches=(), packagesets=(), rebuild=False):
-
+        # Avoid circular imports
+        from lp.registry.model.distroseries import DistroSeries
         self.distroseries = distroseries
         self.parent = self.distroseries.parent_series
         self.arches = arches