Merge ~ines-almeida/launchpad:cleanup-after-pro-enable-migration into launchpad:master

[Ines Almeida]

Cleanup after snap.pro_enable migration

The migration has finishied months ago. This removes the garbo jobs and the auxiliary code that was added to aid the migration.
We also set the 'pro_enable' attribute as non-null now that there are no longer any null values in our databases.

All tests in `snappy.tests` have run successfully.

This is related to this DB MP: https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/457147

And to this Jira ticket: https://warthogs.atlassian.net/browse/LP-1350

I will request for the following query to be run in production before merging this MP, for a last sanity check:
`SELECT pro_enable, Count(*) FROM Snap GROUP BY pro_enable;`

This query returned no null values in December 8th 2023.

[Jürgen Gmach (jugmac00)]

review: Approve

diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py
index 61cd51b..d4fa136 100644
--- a/lib/lp/scripts/garbo.py
+++ b/lib/lp/scripts/garbo.py
@@ -116,8 +116,6 @@ 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 (
@@ -2261,35 +2259,6 @@ 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."""
 
@@ -2626,7 +2595,6 @@ 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 8aee9c5..a5a16e0 100644
--- a/lib/lp/scripts/tests/test_garbo.py
+++ b/lib/lp/scripts/tests/test_garbo.py
@@ -60,7 +60,6 @@ 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
@@ -2585,47 +2584,6 @@ 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 8364c58..722d6d1 100644
--- a/lib/lp/snappy/interfaces/snap.py
+++ b/lib/lp/snappy/interfaces/snap.py
@@ -1301,17 +1301,6 @@ 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 cf18262..8553b3e 100644
--- a/lib/lp/snappy/model/snap.py
+++ b/lib/lp/snappy/model/snap.py
@@ -67,7 +67,6 @@ from lp.code.errors import (
 )
 from lp.code.interfaces.branch import IBranch
 from lp.code.interfaces.branchcollection import IAllBranches, IBranchCollection
-from lp.code.interfaces.branchhosting import InvalidRevisionException
 from lp.code.interfaces.gitcollection import (
     IAllGitRepositories,
     IGitCollection,
@@ -130,7 +129,6 @@ from lp.services.job.model.job import Job
 from lp.services.librarian.model import LibraryFileAlias, LibraryFileContent
 from lp.services.openid.adapters.openid import CurrentOpenIDEndPoint
 from lp.services.propertycache import cachedproperty, get_property_cache
-from lp.services.timeout import default_timeout
 from lp.services.webapp.authorization import precache_permission_for_objects
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.publisher import canonical_url
@@ -394,7 +392,7 @@ class Snap(StormBase, WebhookTargetMixin):
 
     _store_channels = JSON("store_channels", allow_none=True)
 
-    _pro_enable = Bool(name="pro_enable", allow_none=True)
+    pro_enable = Bool(name="pro_enable", allow_none=False)
 
     _use_fetch_service = Bool(name="use_fetch_service", allow_none=False)
 
@@ -458,7 +456,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
+        self.pro_enable = pro_enable
         self.use_fetch_service = use_fetch_service
 
     def __repr__(self):
@@ -687,18 +685,6 @@ 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
-
     @property
     def use_fetch_service(self):
         if getFeatureFlag(SNAP_USE_FETCH_SERVICE_FEATURE_FLAG):
@@ -1550,7 +1536,7 @@ class SnapSet:
         store_secrets=None,
         store_channels=None,
         project=None,
-        pro_enable=None,
+        pro_enable=False,
         use_fetch_service=False,
     ):
         """See `ISnapSet`."""
@@ -1608,9 +1594,6 @@ class SnapSet:
         ):
             raise SnapPrivacyMismatch
 
-        if pro_enable is None:
-            pro_enable = self.inferProEnable(branch or git_ref)
-
         store = IPrimaryStore(Snap)
         snap = Snap(
             registrant,
@@ -1674,44 +1657,6 @@ 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:
-            # Ensure there is a reasonable timeout set. Without this, the
-            # default in snap builds would be 'None', which we don't want.
-            with default_timeout(300.0):
-                snapcraft_data = self.getSnapcraftYaml(context)
-        except (
-            MissingSnapcraftYaml,
-            CannotFetchSnapcraftYaml,
-            CannotParseSnapcraftYaml,
-            InvalidRevisionException,
-        ):
-            pass
-        else:
-            base = snapcraft_data.get("base")
-            build_base = snapcraft_data.get("build-base")
-            name = snapcraft_data.get("name")
-            snap_type = snapcraft_data.get("type")
-
-            if build_base is not None:
-                snap_base_name = build_base
-            elif name is not None and snap_type == "base":
-                snap_base_name = name
-            else:
-                snap_base_name = base
-
-            if snap_base_name is None or snap_base_name == "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 7aa8c29..c85839d 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -3725,86 +3725,11 @@ class TestSnapProcessors(TestCaseWithFactory):
             distro_series=self.factory.makeDistroSeries(),
             name=self.factory.getUniqueUnicode("snap-name"),
             git_ref=git_ref,
-            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(7)]
-        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"),
-                (refs[3], b"name: test-snap\nbuild-base: devel\n"),
-                (refs[4], b"name: core\ntype: base\n"),
-                (refs[5], b"name: core18\ntype: base\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 with only build-base
-        self.assertTrue(inferProEnable(refs[4]))  # 'core' snap itself
-        self.assertFalse(inferProEnable(refs[5]))  # 'core18' snap itself
-        self.assertFalse(inferProEnable(refs[6]))  # Snap w/out snapcraft.yaml
-        self.assertFalse(inferProEnable(None))  # Snap w/out ref or branch
-
-    def test_inferProEnable_branches(self):
-        """With a branch as a context, inferProEnable returns the inferred
-        values for pro-enable as expected (see test description above)
-        """
-        branches = [
-            removeSecurityProxy(self.factory.makeBranch()) for _ in range(7)
-        ]
-        blobs = {
-            branch.id: blob
-            for branch, blob in (
-                (branches[0], b"name: test-snap\n"),
-                (branches[1], b"name: test-snap\nbase: core\n"),
-                (branches[2], b"name: test-snap\nbase: core18\n"),
-                (branches[3], b"name: test-snap\nbuild-base: devel\n"),
-                (branches[4], b"name: core\ntype: base\n"),
-                (branches[5], b"name: core18\ntype: base\n"),
-            )
-        }
-        self.useFixture(BranchHostingFixture()).getBlob = (
-            lambda branch_id, *args, **kwargs: blobs.get(branch_id)
-        )
-
-        inferProEnable = getUtility(ISnapSet).inferProEnable
-        self.assertTrue(inferProEnable(branches[0]))  # Snap w no base
-        self.assertTrue(inferProEnable(branches[1]))  # Snap w 'core' base
-        self.assertFalse(inferProEnable(branches[2]))  # Snap w 'core18' base
-        self.assertFalse(inferProEnable(branches[3]))  # Snap w only build-base
-        self.assertTrue(inferProEnable(branches[4]))  # 'core' snap itself
-        self.assertFalse(inferProEnable(branches[5]))  # 'core18' snap itself
-        self.assertFalse(inferProEnable(branches[6]))  # w/out snapcraft.yaml
-        self.assertFalse(inferProEnable(None))  # w/out ref or branch
-
-    def test_inferProEnable_bad_revision(self):
-        """A branch with an invalid revision ID (or an invalid last_scanned_id)
-        will have its pro-enable value set to False.
-        """
-        branch = self.factory.makeBranch()
-        branch.last_scanned_id = "bad/revision"
-        inferProEnable = getUtility(ISnapSet).inferProEnable
-        self.assertFalse(inferProEnable(removeSecurityProxy(branch)))
-
 
 class TestSnapWebservice(TestCaseWithFactory):
     layer = LaunchpadFunctionalLayer