launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #31116
[Merge] ~ines-almeida/launchpad:cleanup-after-pro-enable-migration into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:cleanup-after-pro-enable-migration into launchpad:master.
Commit message:
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.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/466137
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:cleanup-after-pro-enable-migration into launchpad:master.
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
