launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #30252
[Merge] ~ines-almeida/launchpad:webhook-patterns/update-models into launchpad:master
Ines Almeida has proposed merging ~ines-almeida/launchpad:webhook-patterns/update-models into launchpad:master.
Commit message:
Update Webhook and CIBuild models with the new fields for webhook pattern filtering
`ref_pattern` field and `checkGitRefPattern` endpoint added to Webhook model to enable filtering webhook triggers from git repository according to their git refs
`git_refs` added to CIBuild model will be used to store the git refs that originate the builds
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~ines-almeida/launchpad/+git/launchpad/+merge/446948
This is the preparation work needed to filter git repo webhooks: https://warthogs.atlassian.net/browse/LP-1151
This MP adds 3 main things:
- `ref_pattern` field to Webhook model - to store the pattern used to filter webhooks
- `checkGitRefPattern` endpoint to Webhook model - to check if a git ref matches the ref_pattern of a given webhook
- `git_refs` field to CIBuild model - to store the git refs that originate a given build, to later be used to check against the webhooks ref_pattern
The final functionality to actually filter webhooks according to the `ref_pattern` will be added in another MP.
--
Your team Launchpad code reviewers is requested to review the proposed merge of ~ines-almeida/launchpad:webhook-patterns/update-models into launchpad:master.
diff --git a/lib/lp/code/interfaces/cibuild.py b/lib/lp/code/interfaces/cibuild.py
index bac6181..52b7d33 100644
--- a/lib/lp/code/interfaces/cibuild.py
+++ b/lib/lp/code/interfaces/cibuild.py
@@ -105,6 +105,15 @@ class ICIBuildView(IPackageBuildView, IPrivacy):
)
)
+ git_refs = exported(
+ List(
+ TextLine(),
+ title=_("The git references that originated this CI Build."),
+ required=False,
+ readonly=True,
+ )
+ )
+
distro_arch_series = exported(
Reference(
IDistroArchSeries,
@@ -274,6 +283,7 @@ class ICIBuildSet(ISpecificBuildFarmJobSource):
distro_arch_series,
stages,
date_created=DEFAULT,
+ git_refs=None,
):
"""Create an `ICIBuild`."""
@@ -285,7 +295,9 @@ class ICIBuildSet(ISpecificBuildFarmJobSource):
these Git commit IDs.
"""
- def requestBuild(git_repository, commit_sha1, distro_arch_series, stages):
+ def requestBuild(
+ git_repository, commit_sha1, distro_arch_series, stages, git_refs
+ ):
"""Request a CI build.
This checks that the architecture is allowed and that there isn't
diff --git a/lib/lp/code/model/cibuild.py b/lib/lp/code/model/cibuild.py
index 1cf5a19..0634d50 100644
--- a/lib/lp/code/model/cibuild.py
+++ b/lib/lp/code/model/cibuild.py
@@ -14,7 +14,16 @@ from operator import itemgetter
from lazr.lifecycle.event import ObjectCreatedEvent
from storm.databases.postgres import JSON
-from storm.locals import Bool, DateTime, Desc, Int, Reference, Store, Unicode
+from storm.locals import (
+ Bool,
+ DateTime,
+ Desc,
+ Int,
+ List,
+ Reference,
+ Store,
+ Unicode,
+)
from storm.store import EmptyResultSet
from zope.component import getUtility
from zope.event import notify
@@ -171,12 +180,14 @@ def determine_DASes_to_build(configuration, logger=None):
def get_all_commits_for_paths(git_repository, paths):
- return [
- ref.commit_sha1
- for ref in GitRef.findByReposAndPaths(
- [(git_repository, ref_path) for ref_path in paths]
- ).values()
- ]
+ commits = {}
+ for ref in GitRef.findByReposAndPaths(
+ [(git_repository, ref_path) for ref_path in paths]
+ ).values():
+ if ref.commit_sha1 not in commits:
+ commits[ref.commit_sha1] = []
+ commits[ref.commit_sha1].append(ref.path)
+ return commits
def parse_configuration(git_repository, blob):
@@ -204,6 +215,7 @@ class CIBuild(PackageBuildMixin, StormBase):
git_repository = Reference(git_repository_id, "GitRepository.id")
commit_sha1 = Unicode(name="commit_sha1", allow_none=False)
+ git_refs = List(name="git_refs", allow_none=True)
distro_arch_series_id = Int(name="distro_arch_series", allow_none=False)
distro_arch_series = Reference(
@@ -261,12 +273,14 @@ class CIBuild(PackageBuildMixin, StormBase):
builder_constraints,
stages,
date_created=DEFAULT,
+ git_refs=None,
):
"""Construct a `CIBuild`."""
super().__init__()
self.build_farm_job = build_farm_job
self.git_repository = git_repository
self.commit_sha1 = commit_sha1
+ self.git_refs = git_refs
self.distro_arch_series = distro_arch_series
self.processor = processor
self.virtualized = virtualized
@@ -655,6 +669,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
commit_sha1,
distro_arch_series,
stages,
+ git_refs=None,
date_created=DEFAULT,
):
"""See `ICIBuildSet`."""
@@ -674,6 +689,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
),
stages=stages,
date_created=date_created,
+ git_refs=git_refs,
)
store.add(cibuild)
store.flush()
@@ -712,7 +728,12 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
) is not None and self._isBuildableArchitectureAllowed(das)
def requestBuild(
- self, git_repository, commit_sha1, distro_arch_series, stages
+ self,
+ git_repository,
+ commit_sha1,
+ distro_arch_series,
+ stages,
+ git_refs=None,
):
"""See `ICIBuildSet`."""
pocket = PackagePublishingPocket.UPDATES
@@ -726,17 +747,32 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
CIBuild.distro_arch_series == distro_arch_series,
)
if not result.is_empty():
+ # We append the new git_refs to existing builds here to keep the
+ # git_refs list up-to-date, and potentially filter git repository
+ # webhooks by their git refs if the status of the build changes
+ if git_refs:
+ for cibuild in result:
+ if cibuild.git_refs is None:
+ cibuild.git_refs = []
+ cibuild.git_refs.extend(git_refs)
raise CIBuildAlreadyRequested
build = self.new(
- git_repository, commit_sha1, distro_arch_series, stages
+ git_repository, commit_sha1, distro_arch_series, stages, git_refs
)
build.queueBuild()
notify(ObjectCreatedEvent(build))
return build
def _tryToRequestBuild(
- self, git_repository, commit_sha1, configuration, das, stages, logger
+ self,
+ git_repository,
+ commit_sha1,
+ configuration,
+ das,
+ stages,
+ logger,
+ git_refs=None,
):
try:
if logger is not None:
@@ -746,7 +782,9 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
das.distroseries.name,
das.architecturetag,
)
- build = self.requestBuild(git_repository, commit_sha1, das, stages)
+ build = self.requestBuild(
+ git_repository, commit_sha1, das, stages, git_refs
+ )
# Create reports for each individual job in this build so that
# they show up as pending in the web UI. The job names
# generated here should match those generated by
@@ -778,7 +816,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
# getCommits performs a web request!
commits = getUtility(IGitHostingClient).getCommits(
git_repository.getInternalPath(),
- commit_sha1s,
+ list(commit_sha1s),
# XXX cjwatson 2022-01-19: We should also fetch
# debian/.launchpad.yaml (or perhaps make the path a property of
# the repository) once lpci and launchpad-buildd support using
@@ -814,6 +852,7 @@ class CIBuildSet(SpecificBuildFarmJobSourceMixin):
das,
stages[das.architecturetag],
logger,
+ git_refs=commit_sha1s.get(commit["sha1"]),
)
def getByID(self, build_id):
diff --git a/lib/lp/code/model/tests/test_cibuild.py b/lib/lp/code/model/tests/test_cibuild.py
index c49024a..c9c7235 100644
--- a/lib/lp/code/model/tests/test_cibuild.py
+++ b/lib/lp/code/model/tests/test_cibuild.py
@@ -94,7 +94,7 @@ class TestGetAllCommitsForPaths(TestCaseWithFactory):
rv = get_all_commits_for_paths(repository, ref_paths)
- self.assertEqual([], rv)
+ self.assertEqual({}, rv)
def test_one_ref_one_path(self):
repository = self.factory.makeGitRepository()
@@ -104,7 +104,7 @@ class TestGetAllCommitsForPaths(TestCaseWithFactory):
rv = get_all_commits_for_paths(repository, ref_paths)
self.assertEqual(1, len(rv))
- self.assertEqual(ref.commit_sha1, rv[0])
+ self.assertTrue(ref.commit_sha1 in rv)
def test_multiple_refs_and_paths(self):
repository = self.factory.makeGitRepository()
@@ -781,6 +781,56 @@ class TestCIBuildSet(TestCaseWithFactory):
self.assertIsNone(build_queue.builder_constraints)
self.assertEqual(BuildQueueStatus.WAITING, build_queue.status)
+ def test_requestCIBuild_with_git_refs(self):
+ # requestBuild creates a new CIBuild with the given git_refs
+ repository = self.factory.makeGitRepository()
+ commit_sha1 = hashlib.sha1(self.factory.getUniqueBytes()).hexdigest()
+ das = self.factory.makeBuildableDistroArchSeries()
+ stages = [[("build", 0)]]
+
+ git_refs = ["ref/test", "ref/foo"]
+ build = getUtility(ICIBuildSet).requestBuild(
+ repository, commit_sha1, das, stages, git_refs
+ )
+
+ self.assertTrue(ICIBuild.providedBy(build))
+ self.assertThat(
+ build,
+ MatchesStructure.byEquality(
+ git_repository=repository,
+ commit_sha1=commit_sha1,
+ distro_arch_series=das,
+ stages=stages,
+ status=BuildStatus.NEEDSBUILD,
+ ),
+ )
+ store = Store.of(build)
+ store.flush()
+ build_queue = store.find(
+ BuildQueue,
+ BuildQueue._build_farm_job_id
+ == removeSecurityProxy(build).build_farm_job_id,
+ ).one()
+ self.assertProvides(build_queue, IBuildQueue)
+ self.assertTrue(build_queue.virtualized)
+ self.assertIsNone(build_queue.builder_constraints)
+ self.assertEqual(BuildQueueStatus.WAITING, build_queue.status)
+ self.assertEqual(git_refs, build.git_refs)
+
+ # Re-scheduleing a build for the same comit_sha1 raises an error, but
+ # git_refs of the build are updated
+ self.assertRaises(
+ CIBuildAlreadyRequested,
+ getUtility(ICIBuildSet).requestBuild,
+ repository,
+ commit_sha1,
+ das,
+ stages,
+ ["ref/bar"],
+ )
+ git_refs.append("ref/bar")
+ self.assertEqual(git_refs, build.git_refs)
+
def test_requestBuild_score(self):
# CI builds have an initial queue score of 2600.
repository = self.factory.makeGitRepository()
@@ -1022,6 +1072,7 @@ class TestCIBuildSet(TestCaseWithFactory):
)
),
)
+ self.assertEqual(build.git_refs, ref_paths)
def test_requestBuildsForRefs_multiple_architectures(self):
ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
diff --git a/lib/lp/services/webhooks/browser.py b/lib/lp/services/webhooks/browser.py
index bb53917..686d200 100644
--- a/lib/lp/services/webhooks/browser.py
+++ b/lib/lp/services/webhooks/browser.py
@@ -134,6 +134,7 @@ class WebhookAddView(LaunchpadFormView):
event_types=data["event_types"],
active=data["active"],
secret=data["secret"],
+ ref_pattern=data.get("ref_pattern"),
)
self.next_url = canonical_url(webhook)
diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index b00d45f..f1b48b7 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -25,6 +25,7 @@ from lazr.restful.declarations import (
call_with,
export_destructor_operation,
export_factory_operation,
+ export_read_operation,
export_write_operation,
exported,
exported_as_webservice_entry,
@@ -34,7 +35,7 @@ from lazr.restful.declarations import (
from lazr.restful.fields import CollectionField, Reference
from lazr.restful.interface import copy_field
from zope.interface import Attribute, Interface
-from zope.schema import Bool, Choice, Datetime, Dict, Int, List, TextLine
+from zope.schema import Bool, Choice, Datetime, Dict, Int, List, Text, TextLine
from zope.schema.vocabulary import SimpleVocabulary
from lp import _
@@ -176,6 +177,18 @@ class IWebhook(Interface):
)
)
+ ref_pattern = exported(
+ TextLine(
+ title=_("Git ref pattern"),
+ required=False,
+ description=_(
+ "Pattern to match against git-ref/branch name of an event, "
+ "to filter webhook triggers"
+ ),
+ max_length=200,
+ )
+ )
+
def getDelivery(id):
"""Retrieve a delivery by ID, or None if it doesn't exist."""
@@ -195,9 +208,27 @@ class IWebhook(Interface):
def setSecret(secret):
"""Set the secret used to sign deliveries."""
+ @export_read_operation()
+ @operation_parameters(
+ git_ref=Text(title=_("Git reference to compare pattern against"))
+ )
+ @operation_for_version("devel")
+ def checkGitRefPattern(git_ref: str):
+ """Check if a given git ref matches against the webhook's `ref_pattern`
+ if it contains a `ref_pattern` (only Git Repository webhooks can
+ have a `ref_pattern` value)"""
+
class IWebhookSet(Interface):
- def new(target, registrant, delivery_url, event_types, active, secret):
+ def new(
+ target,
+ registrant,
+ delivery_url,
+ event_types,
+ active,
+ secret,
+ ref_pattern=None,
+ ):
"""Create a new webhook."""
def delete(hooks):
@@ -250,7 +281,12 @@ class IWebhookTarget(Interface):
)
@operation_for_version("devel")
def newWebhook(
- registrant, delivery_url, event_types, active=True, secret=None
+ registrant,
+ delivery_url,
+ event_types,
+ active=True,
+ secret=None,
+ ref_pattern=None,
):
"""Create a new webhook."""
diff --git a/lib/lp/services/webhooks/model.py b/lib/lp/services/webhooks/model.py
index bafd604..de83a62 100644
--- a/lib/lp/services/webhooks/model.py
+++ b/lib/lp/services/webhooks/model.py
@@ -12,6 +12,7 @@ import ipaddress
import re
import socket
from datetime import datetime, timedelta, timezone
+from fnmatch import fnmatch
from urllib.parse import urlsplit
import iso8601
@@ -114,6 +115,8 @@ class Webhook(StormBase):
json_data = JSON(name="json_data")
+ ref_pattern = Unicode(allow_none=True)
+
@property
def target(self):
if self.git_repository is not None:
@@ -168,6 +171,11 @@ class Webhook(StormBase):
def destroySelf(self):
getUtility(IWebhookSet).delete([self])
+ def checkGitRefPattern(self, git_ref: str):
+ if not self.ref_pattern:
+ return True
+ return fnmatch(git_ref, self.ref_pattern)
+
@property
def event_types(self):
return (self.json_data or {}).get("event_types", [])
@@ -192,7 +200,14 @@ class WebhookSet:
"""See `IWebhookSet`."""
def new(
- self, target, registrant, delivery_url, event_types, active, secret
+ self,
+ target,
+ registrant,
+ delivery_url,
+ event_types,
+ active,
+ secret,
+ ref_pattern,
):
from lp.charms.interfaces.charmrecipe import ICharmRecipe
from lp.code.interfaces.branch import IBranch
@@ -233,6 +248,7 @@ class WebhookSet:
hook.delivery_url = delivery_url
hook.active = active
hook.secret = secret
+ hook.ref_pattern = ref_pattern
hook.event_types = event_types
IStore(Webhook).add(hook)
IStore(Webhook).flush()
@@ -346,10 +362,22 @@ class WebhookTargetMixin:
return self.valid_webhook_event_types
def newWebhook(
- self, registrant, delivery_url, event_types, active=True, secret=None
+ self,
+ registrant,
+ delivery_url,
+ event_types,
+ active=True,
+ secret=None,
+ ref_pattern=None,
):
return getUtility(IWebhookSet).new(
- self, registrant, delivery_url, event_types, active, secret
+ self,
+ registrant,
+ delivery_url,
+ event_types,
+ active,
+ secret,
+ ref_pattern,
)
diff --git a/lib/lp/services/webhooks/tests/test_model.py b/lib/lp/services/webhooks/tests/test_model.py
index ede094c..77e54ca 100644
--- a/lib/lp/services/webhooks/tests/test_model.py
+++ b/lib/lp/services/webhooks/tests/test_model.py
@@ -71,6 +71,30 @@ class TestWebhook(TestCaseWithFactory):
webhook.event_types = ["foo", [1]]
self.assertContentEqual([], webhook.event_types)
+ def test_checkGitRefPattern(self):
+ # See lib/lp/code/templates/gitrepository-permissions.pt for an
+ # explanation of the wildcards logic
+ git_ref = "origin/head/foo-test"
+ expected_results = {
+ "": True,
+ "*": True,
+ "*foo*": True,
+ "foo": False,
+ "foo*": False,
+ "*foo": False,
+ "*foo?test": True,
+ "*foo[-_.]test": True,
+ "*foo![-]test": False,
+ "*bar*": False,
+ }
+ results = dict()
+ for pattern in expected_results.keys():
+ webhook = self.factory.makeWebhook(ref_pattern=pattern)
+ with admin_logged_in():
+ results[pattern] = webhook.checkGitRefPattern(git_ref)
+
+ self.assertEqual(expected_results, results)
+
class TestWebhookPermissions(TestCaseWithFactory):
layer = DatabaseFunctionalLayer
@@ -98,6 +122,7 @@ class TestWebhookPermissions(TestCaseWithFactory):
expected_get_permissions = {
"launchpad.View": {
"active",
+ "checkGitRefPattern",
"date_created",
"date_last_modified",
"deliveries",
@@ -112,6 +137,7 @@ class TestWebhookPermissions(TestCaseWithFactory):
"secret",
"setSecret",
"target",
+ "ref_pattern",
},
}
webhook = self.factory.makeWebhook()
@@ -128,6 +154,7 @@ class TestWebhookPermissions(TestCaseWithFactory):
"event_types",
"registrant_id",
"secret",
+ "ref_pattern",
},
}
webhook = self.factory.makeWebhook()
diff --git a/lib/lp/services/webhooks/tests/test_webservice.py b/lib/lp/services/webhooks/tests/test_webservice.py
index 810a83d..aa5e99c 100644
--- a/lib/lp/services/webhooks/tests/test_webservice.py
+++ b/lib/lp/services/webhooks/tests/test_webservice.py
@@ -78,6 +78,7 @@ class TestWebhook(TestCaseWithFactory):
"self_link",
"target_link",
"web_link",
+ "ref_pattern",
),
)
diff --git a/lib/lp/testing/factory.py b/lib/lp/testing/factory.py
index ea6937e..dc27199 100644
--- a/lib/lp/testing/factory.py
+++ b/lib/lp/testing/factory.py
@@ -6163,6 +6163,7 @@ class LaunchpadObjectFactory(ObjectFactory):
secret=None,
active=True,
event_types=None,
+ ref_pattern=None,
):
if target is None:
target = self.makeGitRepository()
@@ -6175,6 +6176,7 @@ class LaunchpadObjectFactory(ObjectFactory):
event_types or [],
active,
secret,
+ ref_pattern,
)
def makeSnap(
Follow ups
