launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> 
> === added file 'lib/lp/snappy/interfaces/basesnap.py'
> --- lib/lp/snappy/interfaces/basesnap.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/interfaces/basesnap.py	2019-02-05 12:26:27 +0000
> @@ -0,0 +1,231 @@
> +# Copyright 2019 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Base snap interfaces."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    "BaseSnapDefaultConflict",
> +    "CannotDeleteBaseSnap",
> +    "IBaseSnap",
> +    "IBaseSnapSet",
> +    "NoSuchBaseSnap",
> +    ]
> +
> +import httplib
> +
> +from lazr.restful.declarations import (
> +    call_with,
> +    collection_default_content,
> +    error_status,
> +    export_as_webservice_collection,
> +    export_as_webservice_entry,
> +    export_destructor_operation,
> +    export_factory_operation,
> +    export_read_operation,
> +    export_write_operation,
> +    exported,
> +    operation_for_version,
> +    operation_parameters,
> +    operation_returns_entry,
> +    REQUEST_USER,
> +    )
> +from lazr.restful.fields import Reference
> +from zope.component import getUtility
> +from zope.interface import Interface
> +from zope.schema import (
> +    Bool,
> +    Datetime,
> +    Dict,
> +    Int,
> +    TextLine,
> +    )
> +
> +from lp import _
> +from lp.app.errors import NameLookupFailed
> +from lp.app.validators.name import name_validator
> +from lp.registry.interfaces.distroseries import IDistroSeries
> +from lp.services.fields import (
> +    ContentNameField,
> +    PublicPersonChoice,
> +    Title,
> +    )
> +
> +
> +@error_status(httplib.CONFLICT)
> +class BaseSnapDefaultConflict(Exception):
> +    """A default base snap already exists."""
> +
> +
> +class NoSuchBaseSnap(NameLookupFailed):
> +    """The requested `BaseSnap` does not exist."""
> +
> +    _message_prefix = "No such base snap"
> +
> +
> +@error_status(httplib.BAD_REQUEST)
> +class CannotDeleteBaseSnap(Exception):
> +    """The base snap cannot be deleted at this time."""
> +
> +
> +class BaseSnapNameField(ContentNameField):
> +    """Ensure that `IBaseSnap` has unique names."""
> +
> +    errormessage = _("%s is already in use by another base snap.")
> +
> +    @property
> +    def _content_iface(self):
> +        """See `UniqueField`."""
> +        return IBaseSnap
> +
> +    def _getByName(self, name):
> +        """See `ContentNameField`."""
> +        try:
> +            return getUtility(IBaseSnapSet).getByName(name)
> +        except NoSuchBaseSnap:
> +            return None
> +
> +
> +class IBaseSnapView(Interface):
> +    """`IBaseSnap` attributes that anyone can view."""
> +
> +    id = Int(title=_("ID"), required=True, readonly=True)
> +
> +    date_created = exported(Datetime(
> +        title=_("Date created"), required=True, readonly=True))
> +
> +    registrant = exported(PublicPersonChoice(
> +        title=_("Registrant"), required=True, readonly=True,
> +        vocabulary="ValidPersonOrTeam",
> +        description=_("The person who registered this base snap.")))
> +
> +    is_default = exported(Bool(
> +        title=_("Is default?"), required=True, readonly=True,
> +        description=_(
> +            "Whether this base snap indicates the defaults used for snap "
> +            "builds that do not specify a base snap.")))
> +
> +
> +class IBaseSnapEditableAttributes(Interface):
> +    """`IBaseSnap` attributes that can be edited.
> +
> +    Anyone can view these attributes, but they need launchpad.Edit to change.
> +    """
> +
> +    name = exported(BaseSnapNameField(
> +        title=_("Name"), required=True, readonly=False,
> +        constraint=name_validator))
> +
> +    display_name = exported(TextLine(
> +        title=_("Display name"), required=True, readonly=False))
> +
> +    title = Title(title=_("Title"), required=True, readonly=True)
> +
> +    distro_series = exported(Reference(
> +        IDistroSeries, title=_("Distro series"),
> +        required=True, readonly=False))
> +
> +    channels = exported(Dict(
> +        title=_("Source snap channels"),
> +        key_type=TextLine(), required=True, readonly=False,
> +        description=_(
> +            "A dictionary mapping snap names to channels to use when building "
> +            "snaps that specify this base snap.")))
> +
> +
> +class IBaseSnapEdit(Interface):
> +    """`IBaseSnap` methods that require launchpad.Edit permission."""
> +
> +    def setDefault(value):
> +        """Set whether this base snap is the default.
> +
> +        This is for internal use; the caller should ensure permission to
> +        edit the base snap and should arrange to remove any existing default
> +        first.  Most callers should use `IBaseSnapSet.setDefault` instead.
> +
> +        :param value: True if this base snap should be the default,
> +            otherwise False.
> +        """

Is it useful to have this in the interface? And could it just be a property?

> +
> +    @export_destructor_operation()
> +    @operation_for_version("devel")
> +    def destroySelf():
> +        """Delete the specified base snap.
> +
> +        :raises CannotDeleteBaseSnap: if the base snap cannot be deleted.
> +        """
> +
> +
> +class IBaseSnap(IBaseSnapView, IBaseSnapEditableAttributes):
> +    """A base snap."""
> +
> +    # XXX cjwatson 2019-01-28 bug=760849: "beta" is a lie to get WADL
> +    # generation working.  Individual attributes must set their version to
> +    # "devel".
> +    export_as_webservice_entry(as_of="beta")
> +
> +
> +class IBaseSnapSetEdit(Interface):
> +    """`IBaseSnapSet` methods that require launchpad.Edit permission."""
> +
> +    @call_with(registrant=REQUEST_USER)
> +    @export_factory_operation(
> +        IBaseSnap, ["name", "display_name", "distro_series", "channels"])
> +    @operation_for_version("devel")
> +    def new(registrant, name, display_name, distro_series, channels,
> +            date_created=None):
> +        """Create an `IBaseSnap`."""
> +
> +    @operation_parameters(
> +        base_snap=Reference(
> +            title=_("Base snap"), required=True, schema=IBaseSnap))
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def setDefault(base_snap):
> +        """Set the default base snap.
> +
> +        This will be used to pick the default distro series for snap builds
> +        that do not specify a base.
> +
> +        :param base_snap: An `IBaseSnap`, or None to unset the default base
> +            snap.
> +        """
> +
> +
> +class IBaseSnapSet(IBaseSnapSetEdit):
> +    """Interface representing the set of base snaps."""
> +
> +    export_as_webservice_collection(IBaseSnap)
> +
> +    def __iter__():
> +        """Iterate over `IBaseSnap`s."""
> +
> +    def __getitem__(name):
> +        """Return the `IBaseSnap` with this name."""
> +
> +    @operation_parameters(
> +        name=TextLine(title=_("Base snap name"), required=True))
> +    @operation_returns_entry(IBaseSnap)
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getByName(name):
> +        """Return the `IBaseSnap` with this name.
> +
> +        :raises NoSuchBaseSnap: if no base snap exists with this name.
> +        """
> +
> +    @operation_returns_entry(IBaseSnap)
> +    @export_read_operation()
> +    @operation_for_version("devel")
> +    def getDefault():
> +        """Get the default base snap.
> +
> +        This will be used to pick the default distro series for snap builds
> +        that do not specify a base.
> +        """
> +
> +    @collection_default_content()
> +    def getAll():
> +        """Return all `IBaseSnap`s."""
> 
> === added file 'lib/lp/snappy/model/basesnap.py'
> --- lib/lp/snappy/model/basesnap.py	1970-01-01 00:00:00 +0000
> +++ lib/lp/snappy/model/basesnap.py	2019-02-05 12:26:27 +0000
> @@ -0,0 +1,147 @@
> +# Copyright 2019 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +"""Base snaps."""
> +
> +from __future__ import absolute_import, print_function, unicode_literals
> +
> +__metaclass__ = type
> +__all__ = [
> +    "BaseSnap",
> +    ]
> +
> +import pytz
> +from storm.locals import (
> +    Bool,
> +    DateTime,
> +    Int,
> +    JSON,
> +    Reference,
> +    Store,
> +    Storm,
> +    Unicode,
> +    )
> +from zope.component import getUtility
> +from zope.interface import implementer
> +
> +from lp.services.database.constants import DEFAULT
> +from lp.services.database.interfaces import (
> +    IMasterStore,
> +    IStore,
> +    )
> +from lp.snappy.interfaces.basesnap import (
> +    BaseSnapDefaultConflict,
> +    CannotDeleteBaseSnap,
> +    IBaseSnap,
> +    IBaseSnapSet,
> +    NoSuchBaseSnap,
> +    )
> +
> +
> +@implementer(IBaseSnap)
> +class BaseSnap(Storm):
> +    """See `IBaseSnap`."""
> +
> +    __storm_table__ = "BaseSnap"
> +
> +    id = Int(primary=True)
> +
> +    date_created = DateTime(
> +        name="date_created", tzinfo=pytz.UTC, allow_none=False)
> +
> +    registrant_id = Int(name="registrant", allow_none=False)
> +    registrant = Reference(registrant_id, "Person.id")
> +
> +    name = Unicode(name="name", allow_none=False)
> +
> +    display_name = Unicode(name="display_name", allow_none=False)
> +
> +    distro_series_id = Int(name="distro_series", allow_none=False)
> +    distro_series = Reference(distro_series_id, "DistroSeries.id")
> +
> +    channels = JSON(name="channels", allow_none=False)
> +
> +    is_default = Bool(name="is_default", allow_none=False)
> +
> +    def __init__(self, registrant, name, display_name, distro_series, channels,
> +                 date_created=DEFAULT):
> +        super(BaseSnap, self).__init__()
> +        self.registrant = registrant
> +        self.name = name
> +        self.display_name = display_name
> +        self.distro_series = distro_series
> +        self.channels = channels
> +        self.date_created = date_created
> +        self.is_default = False
> +
> +    @property
> +    def title(self):
> +        """See `IBaseSnap`."""
> +        return self.display_name

What uses this?

> +
> +    def setDefault(self, value):
> +        """See `IBaseSnap`."""
> +        if value:
> +            # Check for an existing default.
> +            existing = getUtility(IBaseSnapSet).getDefault()
> +            if existing is not None and existing != self:
> +                raise BaseSnapDefaultConflict(
> +                    "The default base snap is already set to %s." %
> +                    existing.name)
> +        self.is_default = value
> +
> +    def destroySelf(self):
> +        """See `IBaseSnap`."""
> +        # Guard against unfortunate accidents.
> +        if self.is_default:
> +            raise CannotDeleteBaseSnap("Cannot delete the default base snap.")
> +        Store.of(self).remove(self)
> +
> +
> +@implementer(IBaseSnapSet)
> +class BaseSnapSet:
> +    """See `IBaseSnapSet`."""
> +
> +    def new(self, registrant, name, display_name, distro_series, channels,
> +            date_created=DEFAULT):
> +        """See `IBaseSnapSet`."""
> +        store = IMasterStore(BaseSnap)
> +        base_snap = BaseSnap(
> +            registrant, name, display_name, distro_series, channels,
> +            date_created=date_created)
> +        store.add(base_snap)
> +        return base_snap
> +
> +    def __iter__(self):
> +        """See `IBaseSnapSet`."""
> +        return iter(self.getAll())
> +
> +    def __getitem__(self, name):
> +        """See `IBaseSnapSet`."""
> +        return self.getByName(name)
> +
> +    def getByName(self, name):
> +        """See `IBaseSnapSet`."""
> +        base_snap = IStore(BaseSnap).find(
> +            BaseSnap, BaseSnap.name == name).one()
> +        if base_snap is None:
> +            raise NoSuchBaseSnap(name)
> +        return base_snap
> +
> +    def getDefault(self):
> +        """See `IBaseSnapSet`."""
> +        return IStore(BaseSnap).find(
> +            BaseSnap, BaseSnap.is_default == True).one()

Can probably drop the "== True".

> +
> +    def setDefault(self, base_snap):
> +        """See `IBaseSnapSet`."""
> +        previous = self.getDefault()
> +        if previous != base_snap:
> +            if previous is not None:
> +                previous.setDefault(False)
> +            if base_snap is not None:
> +                base_snap.setDefault(True)

Really not sure BaseSnap.setDefault is pulling its weight. Setting the attributes directly here (when you're already behind the proxy) should work fine, and you'll know the old one is already unset (and the DB will enforce that).

> +
> +    def getAll(self):
> +        """See `IBaseSnapSet`."""
> +        return IStore(BaseSnap).find(BaseSnap).order_by(BaseSnap.name)


-- 
https://code.launchpad.net/~cjwatson/launchpad/base-snap/+merge/362729
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/base-snap into lp:launchpad.