launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30624
[Merge] ~ines-almeida/launchpad:pro-enable-core18/update-models into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:pro-enable-core18/update-models into launchpad:master.
Commit message:
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.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/453932
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.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:pro-enable-core18/update-models into launchpad:master.
diff --git a/lib/lp/scripts/garbo.py b/lib/lp/scripts/garbo.py
index 06da799..e08accd 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 3599ab1..c2138b4 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 253a91d..2fd4b0f 100644
--- a/lib/lp/snappy/tests/test_snap.py
+++ b/lib/lp/snappy/tests/test_snap.py
@@ -276,6 +276,44 @@ 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
+ """
+
+ # XXX ines-almeida 2023-10-18: If an `IntegrityError` is raised in this
+ # test, then pro_enable is now enforced by DB NOT NULL constraint;
+ # inferring a value is no longer necessary, and this test and
+ # `test_inferProEnable` below can be removed
+
+ 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
+
+ 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 +2286,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 +3640,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 7605ed7..b3e6930 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -6209,6 +6209,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
@@ -6289,6 +6290,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
References
