launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:snap-pillar into launchpad:master.

Commit message:
Adding Snap.project to be the (optional) pillar of snaps

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397458

This adds a project to be the optional pillar of Snaps (mandatory for private Snaps). Once we create the UI to set this, we should start validating and only allowing private Snaps that belongs to a given pillar (so we can use the pillar's sharing options to control Snap's privacy).

Database patch is available here: https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/397459.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:snap-pillar into launchpad:master.

diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 3835b23..c1b2240 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Snap package interfaces."""
@@ -36,6 +36,7 @@ __all__ = [
     'SnapBuildRequestStatus',
     'SnapNotOwner',
     'SnapPrivacyMismatch',
+    'SnapPrivacyPillarError',
     'SnapPrivateFeatureDisabled',
     ]
 
@@ -98,6 +99,7 @@ from lp.code.interfaces.gitrepository import IGitRepository
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import IPerson
 from lp.registry.interfaces.pocket import PackagePublishingPocket
+from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.role import IHasOwner
 from lp.services.fields import (
     PersonChoice,
@@ -215,6 +217,15 @@ class SnapPrivacyMismatch(Exception):
             "Snap contains private information and cannot be public.")
 
 
+@error_status(http_client.BAD_REQUEST)
+class SnapPrivacyPillarError(Exception):
+    """Private Snaps should be based in a pillar."""
+
+    def __init__(self, message=None):
+        super(SnapPrivacyPillarError, self).__init__(
+            message or "Private Snaps should have a pillar.")
+
+
 class BadSnapSearchContext(Exception):
     """The context is not valid for a snap package search."""
 
@@ -658,6 +669,11 @@ class ISnapEditableAttributes(IHasOwner):
         vocabulary="AllUserTeamsParticipationPlusSelf",
         description=_("The owner of this snap package.")))
 
+    project = exported(ReferenceChoice(
+        title=_('The project that this Snap is associated with.'),
+        schema=IProduct, vocabulary='Product',
+        required=False, readonly=False))
+
     distro_series = exported(Reference(
         IDistroSeries, title=_("Distro Series"),
         required=False, readonly=False,
@@ -868,7 +884,7 @@ class ISnapSet(Interface):
             "git_repository", "git_repository_url", "git_path", "git_ref",
             "auto_build", "auto_build_archive", "auto_build_pocket",
             "private", "store_upload", "store_series", "store_name",
-            "store_channels"])
+            "store_channels", "project"])
     @operation_for_version("devel")
     def new(registrant, owner, distro_series, name, description=None,
             branch=None, git_repository=None, git_repository_url=None,
@@ -876,7 +892,8 @@ class ISnapSet(Interface):
             auto_build_archive=None, auto_build_pocket=None,
             require_virtualized=True, processors=None, date_created=None,
             private=False, store_upload=False, store_series=None,
-            store_name=None, store_secrets=None, store_channels=None):
+            store_name=None, store_secrets=None, store_channels=None,
+            project=None):
         """Create an `ISnap`."""
 
     def exists(owner, name):
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index 7e3d14c..4864cf8 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from __future__ import absolute_import, print_function, unicode_literals
@@ -298,6 +298,9 @@ class Snap(Storm, WebhookTargetMixin):
     owner_id = Int(name='owner', allow_none=False, validator=_validate_owner)
     owner = Reference(owner_id, 'Person.id')
 
+    project_id = Int(name='project', allow_none=True)
+    project = Reference(project_id, 'Product.id')
+
     distro_series_id = Int(name='distro_series', allow_none=True)
     distro_series = Reference(distro_series_id, 'DistroSeries.id')
 
@@ -345,6 +348,11 @@ class Snap(Storm, WebhookTargetMixin):
     require_virtualized = Bool(name='require_virtualized')
 
     def _validate_private(self, attr, value):
+        # XXX 2021-02-03: Once we have the browser interface for that,
+        # we should not allow setting Snap.private to True without setting a
+        # pillar. A validation like this:
+        # if value and not self.pillar:
+        #     raise SnapPrivacyPillarError
         if not getUtility(ISnapSet).isValidPrivacy(
                 value, self.owner, self.branch, self.git_ref):
             raise SnapPrivacyMismatch
@@ -374,12 +382,14 @@ class Snap(Storm, WebhookTargetMixin):
                  date_created=DEFAULT, private=False, allow_internet=True,
                  build_source_tarball=False, store_upload=False,
                  store_series=None, store_name=None, store_secrets=None,
-                 store_channels=None):
+                 store_channels=None, project=None):
         """Construct a `Snap`."""
         super(Snap, self).__init__()
 
         # Set the private flag first so that other validators can perform
-        # suitable privacy checks.
+        # suitable privacy checks, but pillar should also be set, since it's
+        # mandatory for private snaps.
+        self.project = project
         self.private = private
 
         self.registrant = registrant
@@ -461,6 +471,21 @@ class Snap(Storm, WebhookTargetMixin):
             return None
 
     @property
+    def pillar(self):
+        """See `ISnap`."""
+        return self.project if self.project_id else None
+
+    @pillar.setter
+    def pillar(self, pillar):
+        if pillar is None:
+            self.project = None
+        elif IProduct.providedBy(pillar):
+            self.project = pillar
+        else:
+            raise ValueError(
+                'The pillar of a Snap must be an IProduct instance.')
+
+    @property
     def available_processors(self):
         """See `ISnap`."""
         clauses = [Processor.id == DistroArchSeries.processor_id]
@@ -1098,7 +1123,7 @@ class SnapSet:
             processors=None, date_created=DEFAULT, private=False,
             allow_internet=True, build_source_tarball=False,
             store_upload=False, store_series=None, store_name=None,
-            store_secrets=None, store_channels=None):
+            store_secrets=None, store_channels=None, project=None):
         """See `ISnapSet`."""
         if not registrant.inTeam(owner):
             if owner.is_team:
@@ -1150,7 +1175,7 @@ class SnapSet:
             build_source_tarball=build_source_tarball,
             store_upload=store_upload, store_series=store_series,
             store_name=store_name, store_secrets=store_secrets,
-            store_channels=store_channels)
+            store_channels=store_channels, project=project)
         store.add(snap)
 
         if processors is None:
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 706255a..2696ff3 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -1,4 +1,4 @@
-# Copyright 2015-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2015-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test snap packages."""
@@ -1421,6 +1421,7 @@ class TestSnapSet(TestCaseWithFactory):
         [ref] = self.factory.makeGitRefs()
         components = self.makeSnapComponents(git_ref=ref)
         components['private'] = True
+        components['project'] = self.factory.makeProduct()
         snap = getUtility(ISnapSet).new(**components)
         with person_logged_in(components['owner']):
             self.assertTrue(snap.private)
@@ -2472,7 +2473,7 @@ class TestSnapWebservice(TestCaseWithFactory):
     def makeSnap(self, owner=None, distroseries=None, branch=None,
                  git_ref=None, processors=None, webservice=None,
                  private=False, auto_build_archive=None,
-                 auto_build_pocket=None, **kwargs):
+                 auto_build_pocket=None, project=None, **kwargs):
         if owner is None:
             owner = self.person
         if distroseries is None:
@@ -2495,6 +2496,8 @@ class TestSnapWebservice(TestCaseWithFactory):
             kwargs["auto_build_archive"] = api_url(auto_build_archive)
         if auto_build_pocket is not None:
             kwargs["auto_build_pocket"] = auto_build_pocket.title
+        if project is not None:
+            kwargs["project"] = api_url(project)
         logout()
         response = webservice.named_post(
             "/+snaps", "new", owner=owner_url, distro_series=distroseries_url,
@@ -2555,6 +2558,7 @@ class TestSnapWebservice(TestCaseWithFactory):
         # Ensure private Snap creation works.
         team = self.factory.makeTeam(owner=self.person)
         distroseries = self.factory.makeDistroSeries(registrant=team)
+        project = self.factory.makeProduct()
         [ref] = self.factory.makeGitRefs()
         private_webservice = webservice_for_person(
             self.person, permission=OAuthPermission.WRITE_PRIVATE)
@@ -2562,9 +2566,10 @@ class TestSnapWebservice(TestCaseWithFactory):
         login(ANONYMOUS)
         snap = self.makeSnap(
             owner=team, distroseries=distroseries, git_ref=ref,
-            webservice=private_webservice, private=True)
+            webservice=private_webservice, private=True, project=project)
         with person_logged_in(self.person):
             self.assertTrue(snap["private"])
+            self.assertEndsWith(snap["project_link"], api_url(project))
 
     def test_new_store_options(self):
         # Ensure store-related options in Snap.new work.
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index c833ddf..9914cc8 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -2,7 +2,7 @@
 # NOTE: The first line above must stay first; do not move the copyright
 # notice to the top.  See http://www.python.org/dev/peps/pep-0263/.
 #
-# Copyright 2009-2020 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2021 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Testing infrastructure for the Launchpad application.
@@ -4754,7 +4754,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                  date_created=DEFAULT, private=False, allow_internet=True,
                  build_source_tarball=False, store_upload=False,
                  store_series=None, store_name=None, store_secrets=None,
-                 store_channels=None):
+                 store_channels=None, project=_DEFAULT):
         """Make a new Snap."""
         if registrant is None:
             registrant = self.makePerson()
@@ -4772,6 +4772,12 @@ class BareLaunchpadObjectFactory(ObjectFactory):
                     distribution=distroseries.distribution, owner=owner)
             if auto_build_pocket is None:
                 auto_build_pocket = PackagePublishingPocket.UPDATES
+        if private and project is _DEFAULT:
+            # If we are creating a private snap and didn't explictly set a
+            # pillar for it, we must create a pillar.
+            project = self.makeProduct()
+        if project is _DEFAULT:
+            project = None
         snap = getUtility(ISnapSet).new(
             registrant, owner, distroseries, name,
             require_virtualized=require_virtualized, processors=processors,
@@ -4783,7 +4789,7 @@ class BareLaunchpadObjectFactory(ObjectFactory):
             build_source_tarball=build_source_tarball,
             store_upload=store_upload, store_series=store_series,
             store_name=store_name, store_secrets=store_secrets,
-            store_channels=store_channels)
+            store_channels=store_channels, project=project)
         if is_stale is not None:
             removeSecurityProxy(snap).is_stale = is_stale
         IStore(snap).flush()