diff --git a/releasenotes/notes/github-checks-dequeue-643b7f757ca4d49b.yaml b/releasenotes/notes/github-checks-dequeue-643b7f757ca4d49b.yaml new file mode 100644 index 0000000000..ba95c7ece4 --- /dev/null +++ b/releasenotes/notes/github-checks-dequeue-643b7f757ca4d49b.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + Changes can now be dequeued via the Github checks API. If a github + reporter is configured to use the checks API, all running checks will + provide a custom "Abort" action. diff --git a/tests/base.py b/tests/base.py index b6721ccbb3..2276673f2f 100644 --- a/tests/base.py +++ b/tests/base.py @@ -2215,6 +2215,31 @@ class FakeGithubPullRequest(object): } return (name, data) + def getCheckRunAbortEvent(self, check_run): + # A check run aborted event can only be created from a FakeCheckRun as + # we need some information like external_id which is "calculated" + # during the creation of the check run. + name = "check_run" + data = { + "action": "requested_action", + "requested_action": { + "identifier": "abort", + }, + "check_run": { + "head_sha": self.head_sha, + "name": check_run["name"], + "app": { + "slug": check_run["app"] + }, + "external_id": check_run["external_id"], + }, + "repository": { + "full_name": self.project, + }, + } + + return (name, data) + def setMerged(self, commit_message): self.is_merged = True self.merge_message = commit_message diff --git a/tests/fakegithub.py b/tests/fakegithub.py index bc94326f36..a18096177d 100644 --- a/tests/fakegithub.py +++ b/tests/fakegithub.py @@ -84,12 +84,17 @@ class FakeStatus(object): class FakeCheckRun(object): def __init__(self, name, details_url, output, status, conclusion, - completed_at, app): + completed_at, external_id, actions, app): + if actions is None: + actions = [] + self.name = name self.details_url = details_url self.output = output self.conclusion = conclusion self.completed_at = completed_at + self.external_id = external_id + self.actions = actions self.app = app # Github automatically sets the status to "completed" if a conclusion @@ -107,16 +112,21 @@ class FakeCheckRun(object): "details_url": self.details_url, "conclusion": self.conclusion, "completed_at": self.completed_at, + "external_id": self.external_id, + "actions": self.actions, "app": { "slug": self.app, }, } - def update(self, conclusion, completed_at, output, details_url): + def update(self, conclusion, completed_at, output, details_url, + external_id, actions): self.conclusion = conclusion self.completed_at = completed_at self.output = output self.details_url = details_url + self.external_id = external_id + self.actions = actions # As we are only calling the update method when a build is completed, # we can always set the status to "completed". @@ -152,9 +162,17 @@ class FakeCommit(object): self._statuses.insert(0, status) def set_check_run(self, name, details_url, output, status, conclusion, - completed_at, app): + completed_at, external_id, actions, app): check_run = FakeCheckRun( - name, details_url, output, status, conclusion, completed_at, app + name, + details_url, + output, + status, + conclusion, + completed_at, + external_id, + actions, + app, ) # Always insert a check_run to the front of the list to represent the # last check_run provided for a commit. @@ -264,7 +282,7 @@ class FakeRepository(object): def create_check_run(self, head_sha, name, details_url=None, output=None, status=None, conclusion=None, completed_at=None, - app="zuul"): + external_id=None, actions=None, app="zuul"): # Raise the appropriate github3 exception in case we don't have # permission to access the checks API @@ -280,7 +298,16 @@ class FakeRepository(object): commit = FakeCommit(head_sha) self._commits[head_sha] = commit commit.set_check_run( - name, details_url, output, status, conclusion, completed_at, app) + name, + details_url, + output, + status, + conclusion, + completed_at, + external_id, + actions, + app, + ) def commit(self, sha): diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 557e587b09..993dc74da0 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import json import os import re from testtools.matchers import MatchesRegex, Not, StartsWith @@ -1796,6 +1797,28 @@ class TestGithubAppDriver(ZuulGithubAppTestCase): check_run["output"]["summary"], MatchesRegex(r'.*Starting checks-api-reporting jobs.*', re.DOTALL) ) + # The external id should be a json-string containing all relevant + # information to uniquely identify this change. + self.assertEqual( + json.dumps( + { + "tenant": "tenant-one", + "pipeline": "checks-api-reporting", + "change": 1 + } + ), + check_run["external_id"], + ) + # A running check run should provide a custom abort action + self.assertEqual(1, len(check_run["actions"])) + self.assertEqual( + { + "identifier": "abort", + "description": "Abort this check run", + "label": "Abort", + }, + check_run["actions"][0], + ) # TODO (felix): How can we test if the details_url was set correctly? # How can the details_url be configured on the test case? @@ -1817,6 +1840,8 @@ class TestGithubAppDriver(ZuulGithubAppTestCase): MatchesRegex(r'.*Build succeeded.*', re.DOTALL) ) self.assertIsNotNone(check_run["completed_at"]) + # A completed check run should not provide any custom actions + self.assertEqual(0, len(check_run["actions"])) # Tell gate to merge to test checks requirements self.fake_github.emitEvent(A.getCommentAddedEvent('merge me')) @@ -1915,6 +1940,8 @@ class TestGithubAppDriver(ZuulGithubAppTestCase): MatchesRegex(r'.*Build succeeded.*', re.DOTALL) ) self.assertIsNotNone(check_run["completed_at"]) + # A completed check run should not provide any custom actions + self.assertEqual(0, len(check_run["actions"])) @simple_layout("layouts/reporting-github.yaml", driver="github") def test_update_check_run_missing_permissions(self): @@ -1950,6 +1977,65 @@ class TestGithubAppDriver(ZuulGithubAppTestCase): self.assertIn(expected_warning, A.comments[0]) self.assertIn(expected_warning, A.comments[1]) + @simple_layout("layouts/reporting-github.yaml", driver="github") + def test_abort_check_run(self): + "Test that we can dequeue a change by aborting the related check run" + project = "org/project3" + + self.executor_server.hold_jobs_in_build = True + A = self.fake_github.openFakePullRequest(project, "master", "A") + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + # We should have a pending check for the head sha that provides an + # abort action. + check_runs = self.fake_github.getCommitChecks(project, A.head_sha) + + self.assertEqual(1, len(check_runs)) + check_run = check_runs[0] + self.assertEqual("tenant-one/checks-api-reporting", check_run["name"]) + self.assertEqual("in_progress", check_run["status"]) + self.assertEqual(1, len(check_run["actions"])) + self.assertEqual("abort", check_run["actions"][0]["identifier"]) + self.assertEqual( + { + "tenant": "tenant-one", + "pipeline": "checks-api-reporting", + "change": 1 + }, + json.loads(check_run["external_id"]) + ) + + # Simulate a click on the "Abort" button in Github by faking a webhook + # event with our custom abort action. + # Handling this event should dequeue the related change + self.fake_github.emitEvent(A.getCheckRunAbortEvent(check_run)) + self.waitUntilSettled() + + tenant = self.scheds.first.sched.abide.tenants.get("tenant-one") + check_pipeline = tenant.layout.pipelines["check"] + self.assertEqual(0, len(check_pipeline.getAllItems())) + self.assertEqual(1, self.countJobResults(self.history, "ABORTED")) + + # The buildset was already dequeued, so there shouldn't be anything to + # release. + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + # Since the change/buildset was dequeued, the check run should be + # reported as cancelled and don't provide any further action. + check_runs = self.fake_github.getCommitChecks(project, A.head_sha) + self.assertEqual(1, len(check_runs)) + aborted_check_run = check_runs[0] + + self.assertEqual( + "tenant-one/checks-api-reporting", aborted_check_run["name"] + ) + self.assertEqual("completed", aborted_check_run["status"]) + self.assertEqual("cancelled", aborted_check_run["conclusion"]) + self.assertEqual(0, len(aborted_check_run["actions"])) + class TestCheckRunAnnotations(ZuulGithubAppTestCase, AnsibleZuulTestCase): """We need Github app authentication and be able to run Ansible jobs""" diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index df3cb50406..ead061e54f 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -46,6 +46,7 @@ from zuul.lib.logutil import get_annotated_logger from zuul.model import Ref, Branch, Tag, Project from zuul.exceptions import MergeFailure from zuul.driver.github.githubmodel import PullRequest, GithubTriggerEvent +from zuul.scheduler import DequeueEvent GITHUB_BASE_URL = 'https://api.github.com' PREVIEW_JSON_ACCEPT = 'application/vnd.github.machine-man-preview+json' @@ -361,6 +362,8 @@ class GithubEventProcessor(object): self.connection.getEventQueueSize()) try: self._process_event() + except Exception: + self.log.exception("Exception when processing event:") finally: self.log.debug("Finished event processing") return self.event @@ -426,7 +429,7 @@ class GithubEventProcessor(object): # If this event references a branch and we're excluding # unprotected branches, we might need to check whether the # branch is now protected. - if event.branch: + if hasattr(event, "branch") and event.branch: b = self.connection.getBranch(project.name, event.branch) if b is not None: branch_protected = b.get('protected') @@ -591,7 +594,7 @@ class GithubEventProcessor(object): # sent by Github whenever a change is pushed. But as we are already # listening to push events, this would result in two trigger events # for the same Github event. - if action not in ["rerequested", "completed"]: + if action not in ["rerequested", "completed", "requested_action"]: return # The head_sha identifies the commit the check_run is requested for @@ -617,16 +620,55 @@ class GithubEventProcessor(object): ) return - # Build a trigger event for the check_run request + # In case a "requested_action" event was triggered, we must first + # evaluate the contained action (identifier), to build the right + # event that will e.g. abort/dequeue a buildset. + if action == "requested_action": + # Look up the action's identifier from the payload + identifier = self.body.get("requested_action", {}).get( + "identifier" + ) + + # currently we only support "abort" identifier + if identifier != "abort": + return + + # In case of an abort (which is currently the only supported + # action), we will build a dequeue event and return this rather + # than a trigger event. + return self._check_run_action_to_event(check_run, project) + + # If no requested_action was supplied, we build a trigger event for the + # check run request event = self._pull_request_to_event(pr_body) event.type = "check_run" + # Simplify rerequested action to requested if action == "rerequested": action = "requested" event.action = action - check_run = "%s:%s:%s" % _check_as_tuple(self.body["check_run"]) - event.check_run = check_run + check_run_tuple = "%s:%s:%s" % _check_as_tuple(check_run) + event.check_run = check_run_tuple + return event + + def _check_run_action_to_event(self, check_run, project): + # Extract necessary values from the check's external id to dequeue + # the corresponding change in Zuul + dequeue_attrs = json.loads(check_run["external_id"]) + # The dequeue operations needs the change in format + # , + change = "{},{}".format(dequeue_attrs["change"], check_run["head_sha"]) + + # Instead of a trigger event, we directly dequeue the change by calling + # the appropriate method on the scheduler. + event = DequeueEvent( + dequeue_attrs["tenant"], + dequeue_attrs["pipeline"], + project, + change, + ref=None + ) return event def _issue_to_pull_request(self, body): @@ -1880,12 +1922,16 @@ class GithubConnection(BaseConnection): log.debug("Removed label %s from %s#%s", label, proj, pr_number) def updateCheck(self, project, pr_number, sha, status, completed, context, - details_url, message, file_comments, zuul_event_id=None): + details_url, message, file_comments, external_id, + zuul_event_id=None): log = get_annotated_logger(self.log, zuul_event_id) github = self.getGithubClient(project, zuul_event_id=zuul_event_id) owner, proj = project.split("/") repository = github.repository(owner, proj) + # Always provide an empty list of actions by default + actions = [] + # Track a list of failed check run operations to report back to Github errors = [] @@ -1959,6 +2005,8 @@ class GithubConnection(BaseConnection): completed_at=completed_at, output=output, details_url=details_url, + external_id=external_id, + actions=actions, ) except github3.exceptions.GitHubException as exc: # TODO (felix): Should we retry the check_run creation? @@ -1986,6 +2034,8 @@ class GithubConnection(BaseConnection): completed_at=completed_at, output=output, details_url=details_url, + external_id=external_id, + actions=actions, ) except github3.exceptions.GitHubException as exc: log.error( @@ -2000,6 +2050,19 @@ class GithubConnection(BaseConnection): ) else: + # Add an abort/dequeue action to running check runs + actions.append( + { + "label": "Abort", + "description": "Abort this check run", + # Usually Github wants us to provide an identifier for our + # system here, so we can identify this action. But as zuul + # is already identifying this event based on the check run + # this shouldn't be necessary. + "identifier": "abort", + } + ) + # Report the start of a check run try: check_run = repository.create_check_run( @@ -2008,6 +2071,8 @@ class GithubConnection(BaseConnection): status=status, output=output, details_url=details_url, + external_id=external_id, + actions=actions, ) except github3.exceptions.GitHubException as exc: # TODO (felix): Should we retry the check run creation? diff --git a/zuul/driver/github/githubreporter.py b/zuul/driver/github/githubreporter.py index 40dbaf5bc3..2f8b653f50 100644 --- a/zuul/driver/github/githubreporter.py +++ b/zuul/driver/github/githubreporter.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import json import logging import voluptuous as v import time @@ -234,6 +235,21 @@ class GithubReporter(BaseReporter): # Check for inline comments that can be reported via checks API file_comments = self.getFileComments(item) + # Github allows an external id to be added to a check run. We can use + # this to identify the check run in any custom actions we define. + # To uniquely identify the corresponding buildset in zuul, we need + # tenant, pipeline and change. The buildset's uuid cannot be used + # safely, as it might change e.g. during a gate reset. Fore more + # information, please see Jim's comment on + # https://review.opendev.org/#/c/666258/7 + external_id = json.dumps( + { + "tenant": item.pipeline.tenant.name, + "pipeline": item.pipeline.name, + "change": item.change.number, + } + ) + return self.connection.updateCheck( project, pr_number, @@ -244,6 +260,7 @@ class GithubReporter(BaseReporter): details_url, message, file_comments, + external_id, zuul_event_id=item.event, ) diff --git a/zuul/scheduler.py b/zuul/scheduler.py index 2375157511..df05b6715b 100644 --- a/zuul/scheduler.py +++ b/zuul/scheduler.py @@ -40,7 +40,7 @@ from zuul.lib.logutil import get_annotated_logger from zuul.lib.statsd import get_statsd import zuul.lib.queue import zuul.lib.repl -from zuul.model import Build, HoldRequest, Tenant +from zuul.model import Build, HoldRequest, Tenant, TriggerEvent COMMANDS = ['full-reconfigure', 'smart-reconfigure', 'stop', 'repl', 'norepl'] @@ -456,9 +456,33 @@ class Scheduler(threading.Thread): self.statsd.gauge('zuul.executors.jobs_queued', execute_queue) def addEvent(self, event): + # Check the event type and put it in the corresponding queue + if isinstance(event, TriggerEvent): + return self._addTriggerEvent(event) + + if isinstance(event, ManagementEvent): + return self._addManagementEvent(event) + + if isinstance(event, ResultEvent): + return self._addResultEvent(event) + + self.log.warning( + "Unable to found appropriate queue for event %s", event + ) + + def _addTriggerEvent(self, event): self.trigger_event_queue.put(event) self.wake_event.set() + def _addManagementEvent(self, event): + self.management_event_queue.put(event) + self.wake_event.set() + event.wait() + + def _addResultEvent(self, event): + self.result_event_queue.put(event) + self.wake_event.set() + def onBuildStarted(self, build): build.start_time = time.time() event = BuildStartedEvent(build)