Merge ~ines-almeida/launchpad:pro-enable-core18/update-models into launchpad:master

[Ines Almeida]

Add new pro_enable attribute to Snap models, plus DB backfilling logic

This new bool will later determine whether a snap can use private dependencies.
Initially this attribute is NULL in the database, and will be backfilled daily, a chunk at a time. As such, we add both `_pro_enable` (the actual DB value) and `pro_enable` as a model property that determines the `pro_enable` value if one isn't set in the DB yet.

For more context, see: https://docs.google.com/document/d/19juEP2pOsww4t9Z-jtVZbJdUHgZqkmE9mRukGBQ8DHk

This MP adds the `pro_enable` attribute to the model, and its DB backfilling logic.
What it doesn't add:
 - Browser interface to see or update this value
 - Any logic changes based on this value

This should be able to be merged and start back filling the DB, even if the value isn't at all used yet.

[Colin Watson (cjwatson)]

review: Approve

add27ba... by Ines Almeida on 2023-10-23

Prevent snap pro-enable test from failing once DB restriction patch is applied

diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py
index d4fa136..61cd51b 100644
--- a/lib/lp/scripts/garbo.py
+++ b/lib/lp/scripts/garbo.py
@@ -116,6 +116,8 @@ from lp.services.verification.model.logintoken import LoginToken
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webhooks.interfaces import IWebhookJobSource
 from lp.services.webhooks.model import WebhookJob
+from lp.snappy.interfaces.snap import ISnapSet
+from lp.snappy.model.snap import Snap
 from lp.snappy.model.snapbuild import SnapFile
 from lp.snappy.model.snapbuildjob import SnapBuildJobType
 from lp.soyuz.enums import (
@@ -2259,6 +2261,35 @@ class ArchiveFileDatePopulator(TunableLoop):
         transaction.commit()
 
 
+class SnapProEnablePopulator(TunableLoop):
+    """Populates Snap.pro_enable."""
+
+    maximum_chunk_size = 100
+
+    def __init__(self, log, abort_time=None):
+        super().__init__(log, abort_time)
+        self.start_at = 1
+        self.store = IPrimaryStore(Snap)
+
+    def findSnaps(self):
+        snaps = self.store.find(
+            Snap,
+            Snap.id >= self.start_at,
+            Snap._pro_enable == None,
+        )
+        return snaps.order_by(Snap.id)
+
+    def isDone(self):
+        return self.findSnaps().is_empty()
+
+    def __call__(self, chunk_size):
+        snaps = list(self.findSnaps()[:chunk_size])
+        for snap in snaps:
+            snap._pro_enable = getUtility(ISnapSet).inferProEnable(snap.source)
+        self.start_at = snaps[-1].id + 1
+        transaction.commit()
+
+
 class BaseDatabaseGarbageCollector(LaunchpadCronScript):
     """Abstract base class to run a collection of TunableLoops."""
 
@@ -2595,6 +2626,7 @@ class DailyDatabaseGarbageCollector(BaseDatabaseGarbageCollector):
         ScrubPOFileTranslator,
         SnapBuildJobPruner,
         SnapFilePruner,
+        SnapProEnablePopulator,
         SourcePackagePublishingHistoryFormatPopulator,
         SuggestiveTemplatesCacheUpdater,
         TeamMembershipPruner,
diff --git a/lib/lp/scripts/tests/test_garbo.py b/lib/lp/scripts/tests/test_garbo.py
index e2bc91b..726ba55 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -60,6 +60,7 @@ from lp.code.model.codeimportresult import CodeImportResult
 from lp.code.model.diff import Diff
 from lp.code.model.gitjob import GitJob, GitRefScanJob
 from lp.code.model.gitrepository import GitRepository
+from lp.code.tests.helpers import GitHostingFixture
 from lp.oci.interfaces.ocirecipe import OCI_RECIPE_ALLOW_CREATE
 from lp.oci.model.ocirecipebuild import OCIFile
 from lp.registry.enums import BranchSharingPolicy, BugSharingPolicy, VCSType
@@ -2586,6 +2587,47 @@ class TestGarbo(FakeAdapterMixin, TestCaseWithFactory):
         self.assertEqual(1, rs.count())
         self.assertEqual(archive_files[1], rs.one())
 
+    def test_SnapProEnablePopulator(self):
+        switch_dbuser("testadmin")
+        refs = [self.factory.makeGitRefs()[0] for _ in range(4)]
+        blobs = {
+            ref.repository.getInternalPath(): blob
+            for ref, blob in (
+                (refs[0], b"name: test-snap\n"),
+                (refs[1], b"name: test-snap\nbase: core\n"),
+                (refs[2], b"name: test-snap\nbase: core18\n"),
+            )
+        }
+        self.useFixture(
+            GitHostingFixture()
+        ).getBlob = lambda path, *args, **kwargs: blobs.get(path)
+        old_snaps = [self.factory.makeSnap(git_ref=ref) for ref in refs]
+        for snap in old_snaps:
+            removeSecurityProxy(snap)._pro_enable = None
+        try:
+            Store.of(old_snaps[0]).flush()
+        except IntegrityError:
+            # Now enforced by DB NOT NULL constraint; backfilling is no
+            # longer necessary.
+            return
+
+        new_snaps = [
+            self.factory.makeSnap(pro_enable=False),
+            self.factory.makeSnap(pro_enable=True),
+        ]
+        transaction.commit()
+
+        self.runDaily()
+
+        # Old snap recipes are backfilled.
+        self.assertIs(True, removeSecurityProxy(old_snaps[0])._pro_enable)
+        self.assertIs(True, removeSecurityProxy(old_snaps[1])._pro_enable)
+        self.assertIs(False, removeSecurityProxy(old_snaps[2])._pro_enable)
+        self.assertIs(False, removeSecurityProxy(old_snaps[3])._pro_enable)
+        # Other snap recipes are left alone.
+        self.assertIs(False, removeSecurityProxy(new_snaps[0])._pro_enable)
+        self.assertIs(True, removeSecurityProxy(new_snaps[1])._pro_enable)
+
 
 class TestGarboTasks(TestCaseWithFactory):
     layer = LaunchpadZopelessLayer
diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
index 56f40d0..ae2e3df 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -1177,6 +1177,18 @@ class ISnapAdminAttributes(Interface):
         )
     )
 
+    pro_enable = exported(
+        Bool(
+            title=_("Enable Ubuntu Pro"),
+            required=True,
+            readonly=False,
+            description=_(
+                "Allow building this snap recipe using dependencies from "
+                "Ubuntu Pro, if configured for the corresponding snap base."
+            ),
+        )
+    )
+
     def subscribe(person, subscribed_by):
         """Subscribe a person to this snap recipe."""
 
@@ -1254,6 +1266,7 @@ class ISnapSet(Interface):
         store_secrets=None,
         store_channels=None,
         project=None,
+        pro_enable=None,
     ):
         """Create an `ISnap`."""
 
@@ -1273,6 +1286,17 @@ class ISnapSet(Interface):
     ):
         """Whether or not the information type context is valid."""
 
+    def inferProEnable(context):
+        """Infer a backward-compatible setting of pro_enable.
+
+        New snap recipes only build using dependencies from Ubuntu Pro if
+        explicitly configured to do so, but historically we enabled this by
+        default for snap recipes based on "core" (i.e. Ubuntu Core 16).  For
+        backward compatibility, we continue doing this until we have
+        self-service Pro enablement for snap recipes; see
+        https://docs.google.com/document/d/19juEP2pOsww4t9Z-jtVZbJdUHgZqkmE9mRukGBQ8DHk.
+        """
+
     @operation_parameters(
         owner=Reference(IPerson, title=_("Owner"), required=True),
         name=TextLine(title=_("Snap name"), required=True),
diff --git a/lib/lp/snappy/model/snap.py b/lib/lp/snappy/model/snap.py
index a9a0128..42c1cee 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -390,6 +390,8 @@ class Snap(StormBase, WebhookTargetMixin):
 
     _store_channels = JSON("store_channels", allow_none=True)
 
+    _pro_enable = Bool(name="pro_enable", allow_none=True)
+
     def __init__(
         self,
         registrant,
@@ -414,6 +416,7 @@ class Snap(StormBase, WebhookTargetMixin):
         store_secrets=None,
         store_channels=None,
         project=None,
+        pro_enable=False,
     ):
         """Construct a `Snap`."""
         super().__init__()
@@ -448,6 +451,7 @@ class Snap(StormBase, WebhookTargetMixin):
         self.store_name = store_name
         self.store_secrets = store_secrets
         self.store_channels = store_channels
+        self._pro_enable = pro_enable
 
     def __repr__(self):
         return "<Snap ~%s/+snap/%s>" % (self.owner.name, self.name)
@@ -675,6 +679,18 @@ class Snap(StormBase, WebhookTargetMixin):
     def store_channels(self, value):
         self._store_channels = value or None
 
+    # XXX ines-almeida 2023-10-18: Simplify this once the database column has
+    # been backfilled.
+    @property
+    def pro_enable(self):
+        if self._pro_enable is None:
+            return getUtility(ISnapSet).inferProEnable(self.source)
+        return self._pro_enable
+
+    @pro_enable.setter
+    def pro_enable(self, value):
+        self._pro_enable = value
+
     def getAllowedInformationTypes(self, user):
         """See `ISnap`."""
         if user_has_special_snap_access(user):
@@ -1515,6 +1531,7 @@ class SnapSet:
         store_secrets=None,
         store_channels=None,
         project=None,
+        pro_enable=None,
     ):
         """See `ISnapSet`."""
         if not registrant.inTeam(owner):
@@ -1571,6 +1588,9 @@ class SnapSet:
         ):
             raise SnapPrivacyMismatch
 
+        if pro_enable is None:
+            pro_enable = self.inferProEnable(branch or git_ref)
+
         store = IPrimaryStore(Snap)
         snap = Snap(
             registrant,
@@ -1595,6 +1615,7 @@ class SnapSet:
             store_secrets=store_secrets,
             store_channels=store_channels,
             project=project,
+            pro_enable=pro_enable,
         )
         store.add(snap)
         snap._reconcileAccess()
@@ -1632,6 +1653,29 @@ class SnapSet:
 
         return True
 
+    # XXX ines-almeida 2023-10-18: Remove this once we have self-service Pro
+    # enablement for snap recipes.
+    def inferProEnable(self, context):
+        """See `ISnapSet`."""
+        if context is None:
+            # Recipe has been detached from its source.
+            return False
+
+        try:
+            snapcraft_data = self.getSnapcraftYaml(context)
+        except (
+            MissingSnapcraftYaml,
+            CannotFetchSnapcraftYaml,
+            CannotParseSnapcraftYaml,
+        ):
+            pass
+        else:
+            base = snapcraft_data.get("base")
+            if base is None or base == "core":
+                return True
+
+        return False
+
     def _getByName(self, owner, name):
         return (
             IStore(Snap)
diff --git a/lib/lp/snappy/tests/test_snap.py b/lib/lp/snappy/tests/test_snap.py
index 5acd69f..665d1d8 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -15,6 +15,7 @@ import responses
 import transaction
 from fixtures import FakeLogger, MockPatch
 from nacl.public import PrivateKey
+from psycopg2 import IntegrityError
 from pymacaroons import Macaroon
 from storm.exceptions import LostObjectError
 from storm.locals import Store
@@ -276,6 +277,46 @@ class TestSnap(TestCaseWithFactory):
             pass
         self.assertSqlAttributeEqualsDate(snap, "date_last_modified", UTC_NOW)
 
+    def test_pro_enable_value_for_existing_snaps(self):
+        """For existing snaps without pro-enable values, the value is set as
+        expected once called:
+         - If snap has snapcraft.yaml file, and no base - True
+         - If snap has snapcraft.yaml file, and is 'core'-based snap - True
+         - Else, default to False
+        """
+
+        refs = [self.factory.makeGitRefs()[0] for _ in range(4)]
+        blobs = {
+            ref.repository.getInternalPath(): blob
+            for ref, blob in (
+                (refs[0], b"name: test-snap\n"),
+                (refs[1], b"name: test-snap\nbase: core\n"),
+                (refs[2], b"name: test-snap\nbase: core18\n"),
+            )
+        }
+        self.useFixture(
+            GitHostingFixture()
+        ).getBlob = lambda path, *args, **kwargs: blobs.get(path)
+        snaps = [self.factory.makeSnap(git_ref=ref) for ref in refs]
+        for snap in snaps:
+            removeSecurityProxy(snap)._pro_enable = None
+
+        try:
+            Store.of(snaps[0]).flush()
+        except IntegrityError:
+            # Now enforced by DB NOT NULL constraint; inferring a value is
+            # no longer necessary.
+            return
+
+        self.assertTrue(snaps[0].pro_enable)  # Snap with no base
+        self.assertTrue(removeSecurityProxy(snaps[0])._pro_enable)
+        self.assertTrue(snaps[1].pro_enable)  # Snap with 'core' base
+        self.assertTrue(removeSecurityProxy(snaps[1])._pro_enable)
+        self.assertFalse(snaps[2].pro_enable)  # Snap with 'core18' base
+        self.assertFalse(removeSecurityProxy(snaps[2])._pro_enable)
+        self.assertFalse(snaps[3].pro_enable)  # Snap without snapcraft.yaml
+        self.assertFalse(removeSecurityProxy(snaps[3])._pro_enable)
+
     def makeBuildableDistroArchSeries(self, **kwargs):
         das = self.factory.makeDistroArchSeries(**kwargs)
         fake_chroot = self.factory.makeLibraryFileAlias(
@@ -2248,6 +2289,7 @@ class TestSnapSet(TestCaseWithFactory):
             owner=self.factory.makeTeam(owner=registrant),
             distro_series=self.factory.makeDistroSeries(),
             name=self.factory.getUniqueUnicode("snap-name"),
+            pro_enable=False,
         )
         if branch is None and git_ref is None:
             branch = self.factory.makeAnyBranch()
@@ -3601,6 +3643,48 @@ class TestSnapProcessors(TestCaseWithFactory):
                 [self.default_procs[0], self.arm, hppa], snap.processors
             )
 
+    def test_pro_enabled_default_value_for_new_snap(self):
+        """Snap pro_enable value defaults to False when creating a new Snap."""
+
+        git_ref = self.factory.makeGitRefs()[0]
+        blob = b"name: test-snap\nbase: core18\n"
+        self.useFixture(
+            GitHostingFixture()
+        ).getBlob = lambda path, *args, **kwargs: blob
+
+        components = self.makeSnapComponents(git_ref=git_ref)
+        components["pro_enable"] = None
+
+        snap = getUtility(ISnapSet).new(**components)
+        self.assertFalse(snap.pro_enable)
+
+    def test_inferProEnable(self):
+        """inferProEnable returns expected bool value depending on context:
+        - Context and snapcraft.yaml file exist, and no base - True
+        - Context and snapcraft.yaml file exist, and base is 'core' - True
+        - Else, default to False
+        """
+
+        refs = [self.factory.makeGitRefs()[0] for _ in range(4)]
+        blobs = {
+            ref.repository.getInternalPath(): blob
+            for ref, blob in (
+                (refs[0], b"name: test-snap\n"),
+                (refs[1], b"name: test-snap\nbase: core\n"),
+                (refs[2], b"name: test-snap\nbase: core18\n"),
+            )
+        }
+        self.useFixture(
+            GitHostingFixture()
+        ).getBlob = lambda path, *args, **kwargs: blobs.get(path)
+
+        inferProEnable = getUtility(ISnapSet).inferProEnable
+        self.assertTrue(inferProEnable(refs[0]))  # Snap with no base
+        self.assertTrue(inferProEnable(refs[1]))  # Snap with 'core' base
+        self.assertFalse(inferProEnable(refs[2]))  # Snap with 'core18' base
+        self.assertFalse(inferProEnable(refs[3]))  # Snap w/out snapcraft.yaml
+        self.assertFalse(inferProEnable(None))  # Snap w/out ref or branch
+
 
 class TestSnapWebservice(TestCaseWithFactory):
     layer = LaunchpadFunctionalLayer
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index bfc8744..0e9236e 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -6208,6 +6208,7 @@ class LaunchpadObjectFactory(ObjectFactory):
         store_secrets=None,
         store_channels=None,
         project=_DEFAULT,
+        pro_enable=False,
     ):
         """Make a new Snap."""
         assert information_type is None or private is None
@@ -6288,6 +6289,7 @@ class LaunchpadObjectFactory(ObjectFactory):
             store_secrets=store_secrets,
             store_channels=store_channels,
             project=project,
+            pro_enable=pro_enable,
         )
         if is_stale is not None:
             removeSecurityProxy(snap).is_stale = is_stale