From d96e5887b8b2de3ab000036c5aa355d53ac86de9 Mon Sep 17 00:00:00 2001 From: Jesse Keating Date: Thu, 19 Jan 2017 13:55:50 -0800 Subject: [PATCH] Add support for requiring github pr head status When creating a github pull request change object, the statuses of the head commit are fetched, and the latest status of each user and context set is appended into a list of statuses. The string is 'user:context:state', where state can be success, error, or failure. Context is freeform, and the user is the login of the entity that set the status. Tests have been added to validate that github based requirements on pipelines are honored. Change-Id: I45abbd6cbddd36b8491bdf9bb8d545216537ad2f Signed-off-by: Jesse Keating --- tests/base.py | 50 ++++++++++++++----- tests/fixtures/layouts/reporting-github.yaml | 1 + .../fixtures/layouts/requirements-github.yaml | 23 +++++++++ tests/unit/test_github_driver.py | 24 ++++++--- tests/unit/test_github_requirements.py | 45 +++++++++++++++++ zuul/driver/github/githubconnection.py | 36 +++++++++++++ zuul/driver/github/githubmodel.py | 30 ++++++++++- zuul/driver/github/githubsource.py | 10 +++- 8 files changed, 197 insertions(+), 22 deletions(-) create mode 100644 tests/fixtures/layouts/requirements-github.yaml create mode 100644 tests/unit/test_github_requirements.py diff --git a/tests/base.py b/tests/base.py index 0105ffaaad..b1ef3c9d88 100755 --- a/tests/base.py +++ b/tests/base.py @@ -569,13 +569,11 @@ class FakeGithubPullRequest(object): """Adds a commit on top of the actual PR head.""" self._addCommitToRepo(files=files) self._updateTimeStamp() - self._clearStatuses() def forcePush(self, files=[]): """Clears actual commits and add a commit on top of the base.""" self._addCommitToRepo(files=files, reset=True) self._updateTimeStamp() - self._clearStatuses() def getPullRequestOpenedEvent(self): return self._getPullRequestEvent('opened') @@ -695,7 +693,10 @@ class FakeGithubPullRequest(object): } }, 'head': { - 'sha': self.head_sha + 'sha': self.head_sha, + 'repo': { + 'full_name': self.project + } } }, 'label': { @@ -742,6 +743,9 @@ class FakeGithubPullRequest(object): repo.index.add([fn]) self.head_sha = repo.index.commit(msg).hexsha + # Create an empty set of statuses for the given sha, + # each sha on a PR may have a status set on it + self.statuses[self.head_sha] = [] repo.head.reference = 'master' zuul.merger.merger.reset_repo_to_head(repo) repo.git.clean('-x', '-f', '-d') @@ -754,15 +758,21 @@ class FakeGithubPullRequest(object): repo = self._getRepo() return repo.references[self._getPRReference()].commit.hexsha - def setStatus(self, state, url, description, context): - self.statuses[context] = { + def setStatus(self, sha, state, url, description, context): + # Since we're bypassing github API, which would require a user, we + # hard set the user as 'zuul' here. + user = 'zuul' + # insert the status at the top of the list, to simulate that it + # is the most recent set status + self.statuses[sha].insert(0, ({ 'state': state, 'url': url, - 'description': description - } - - def _clearStatuses(self): - self.statuses = {} + 'description': description, + 'context': context, + 'creator': { + 'login': user + } + })) def _getPRReference(self): return '%s/head' % self.number @@ -783,7 +793,10 @@ class FakeGithubPullRequest(object): } }, 'head': { - 'sha': self.head_sha + 'sha': self.head_sha, + 'repo': { + 'full_name': self.project + } } }, 'sender': { @@ -857,7 +870,10 @@ class FakeGithubConnection(githubconnection.GithubConnection): }, 'mergeable': True, 'head': { - 'sha': pr.head_sha + 'sha': pr.head_sha, + 'repo': { + 'full_name': pr.project + } } } return data @@ -901,6 +917,14 @@ class FakeGithubConnection(githubconnection.GithubConnection): pull_request.is_merged = True pull_request.merge_message = commit_message + def getCommitStatuses(self, project, sha): + owner, proj = project.split('/') + for pr in self.pull_requests: + pr_owner, pr_project = pr.project.split('/') + if (pr_owner == owner and pr_project == proj and + pr.head_sha == sha): + return pr.statuses[sha] + def setCommitStatus(self, project, sha, state, url='', description='', context=''): owner, proj = project.split('/') @@ -908,7 +932,7 @@ class FakeGithubConnection(githubconnection.GithubConnection): pr_owner, pr_project = pr.project.split('/') if (pr_owner == owner and pr_project == proj and pr.head_sha == sha): - pr.setStatus(state, url, description, context) + pr.setStatus(sha, state, url, description, context) def labelPull(self, project, pr_number, label): pull_request = self.pull_requests[pr_number - 1] diff --git a/tests/fixtures/layouts/reporting-github.yaml b/tests/fixtures/layouts/reporting-github.yaml index bcbac1bde9..c939f39950 100644 --- a/tests/fixtures/layouts/reporting-github.yaml +++ b/tests/fixtures/layouts/reporting-github.yaml @@ -28,6 +28,7 @@ success: github: comment: false + status: 'success' failure: github: comment: false diff --git a/tests/fixtures/layouts/requirements-github.yaml b/tests/fixtures/layouts/requirements-github.yaml new file mode 100644 index 0000000000..cacc54f294 --- /dev/null +++ b/tests/fixtures/layouts/requirements-github.yaml @@ -0,0 +1,23 @@ +- pipeline: + name: pipeline + manager: independent + require: + github: + status: "zuul:check:success" + trigger: + github: + - event: pull_request + action: comment + comment: 'test me' + success: + github: + comment: true + +- job: + name: project1-pipeline + +- project: + name: org/project1 + pipeline: + jobs: + - project1-pipeline diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index f9182186c7..f2877903c5 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -253,10 +253,14 @@ class TestGithubDriver(ZuulTestCase): A = self.fake_github.openFakePullRequest('org/project', 'master', 'A') self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) self.waitUntilSettled() - self.assertIn('check', A.statuses) - check_status = A.statuses['check'] + # We should have a status container for the head sha + self.assertIn(A.head_sha, A.statuses.keys()) + # We should only have one status for the head sha + self.assertEqual(1, len(A.statuses[A.head_sha])) + check_status = A.statuses[A.head_sha][0] check_url = ('http://zuul.example.com/status/#%s,%s' % (A.number, A.head_sha)) + self.assertEqual('check', check_status['context']) self.assertEqual('Standard check', check_status['description']) self.assertEqual('pending', check_status['state']) self.assertEqual(check_url, check_status['url']) @@ -265,8 +269,12 @@ class TestGithubDriver(ZuulTestCase): self.executor_server.hold_jobs_in_build = False self.executor_server.release() self.waitUntilSettled() - check_status = A.statuses['check'] - self.assertEqual('Standard check', check_status['description']) + # We should only have two statuses for the head sha + self.assertEqual(2, len(A.statuses[A.head_sha])) + check_status = A.statuses[A.head_sha][0] + check_url = ('http://zuul.example.com/status/#%s,%s' % + (A.number, A.head_sha)) + self.assertEqual('check', check_status['context']) self.assertEqual('success', check_status['state']) self.assertEqual(check_url, check_status['url']) self.assertEqual(1, len(A.comments)) @@ -278,7 +286,7 @@ class TestGithubDriver(ZuulTestCase): self.fake_github.emitEvent( A.getCommentAddedEvent('reporting check')) self.waitUntilSettled() - self.assertNotIn('reporting', A.statuses) + self.assertEqual(2, len(A.statuses[A.head_sha])) # comments increased by one for the start message self.assertEqual(2, len(A.comments)) self.assertThat(A.comments[1], @@ -286,7 +294,11 @@ class TestGithubDriver(ZuulTestCase): self.executor_server.hold_jobs_in_build = False self.executor_server.release() self.waitUntilSettled() - self.assertNotIn('reporting', A.statuses) + # pipeline reports success status + self.assertEqual(3, len(A.statuses[A.head_sha])) + report_status = A.statuses[A.head_sha][0] + self.assertEqual('reporting', report_status['context']) + self.assertEqual('success', report_status['state']) self.assertEqual(2, len(A.comments)) @simple_layout('layouts/merging-github.yaml', driver='github') diff --git a/tests/unit/test_github_requirements.py b/tests/unit/test_github_requirements.py new file mode 100644 index 0000000000..bb9993e71f --- /dev/null +++ b/tests/unit/test_github_requirements.py @@ -0,0 +1,45 @@ +#!/usr/bin/env python +# Copyright (c) 2017 IBM Corp. +# +# Licensed under the Apache License, Version 2.0 (the "License"); you may +# not use this file except in compliance with the License. You may obtain +# a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +# License for the specific language governing permissions and limitations +# under the License. + +from tests.base import ZuulTestCase, simple_layout + + +class TestGithubRequirements(ZuulTestCase): + """Test pipeline and trigger requirements""" + config_file = 'zuul-github-driver.conf' + + @simple_layout('layouts/requirements-github.yaml', driver='github') + def test_pipeline_require_status(self): + "Test pipeline requirement: status" + A = self.fake_github.openFakePullRequest('org/project1', 'master', 'A') + # A comment event that we will keep submitting to trigger + comment = A.getCommentAddedEvent('test me') + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + # No status from zuul so should not be enqueued + self.assertEqual(len(self.history), 0) + + # An error status should not cause it to be enqueued + A.setStatus(A.head_sha, 'error', 'null', 'null', 'check') + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 0) + + # A success status goes in + A.setStatus(A.head_sha, 'success', 'null', 'null', 'check') + self.fake_github.emitEvent(comment) + self.waitUntilSettled() + self.assertEqual(len(self.history), 1) + self.assertEqual(self.history[0].name, 'project1-pipeline') diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 4b945a53aa..2513569575 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -315,6 +315,7 @@ class GithubConnection(BaseConnection): change.patchset = event.patch_number change.files = self.getPullFileNames(project, change.number) change.title = event.title + change.status = self._get_statuses(project, event.patch_number) change.source_event = event elif event.ref: change = Ref(project) @@ -408,6 +409,17 @@ class GithubConnection(BaseConnection): if not result: raise Exception('Pull request was not merged') + def getCommitStatuses(self, project, sha): + owner, proj = project.split('/') + repository = self.github.repository(owner, proj) + commit = repository.commit(sha) + # make a list out of the statuses so that we complete our + # API transaction + statuses = [status.as_dict() for status in commit.statuses()] + + log_rate_limit(self.log, self.github) + return statuses + def setCommitStatus(self, project, sha, state, url='', description='', context=''): owner, proj = project.split('/') @@ -430,6 +442,30 @@ class GithubConnection(BaseConnection): def _ghTimestampToDate(self, timestamp): return time.strptime(timestamp, '%Y-%m-%dT%H:%M:%SZ') + def _get_statuses(self, project, sha): + # A ref can have more than one status from each context, + # however the API returns them in order, newest first. + # So we can keep track of which contexts we've already seen + # and throw out the rest. Our unique key is based on + # the user and the context, since context is free form and anybody + # can put whatever they want there. We want to ensure we track it + # by user, so that we can require/trigger by user too. + seen = [] + statuses = [] + for status in self.getCommitStatuses(project.name, sha): + # creator can be None if the user has been removed. + creator = status.get('creator') + if not creator: + continue + user = creator.get('login') + context = status.get('context') + state = status.get('state') + if "%s:%s" % (user, context) not in seen: + statuses.append("%s:%s:%s" % (user, context, state)) + seen.append("%s:%s" % (user, context)) + + return statuses + def log_rate_limit(log, github): try: diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py index 0d77cae349..22f549fa9b 100644 --- a/zuul/driver/github/githubmodel.py +++ b/zuul/driver/github/githubmodel.py @@ -16,7 +16,7 @@ import re -from zuul.model import Change, TriggerEvent, EventFilter +from zuul.model import Change, TriggerEvent, EventFilter, RefFilter EMPTY_GIT_REF = '0' * 40 # git sha of all zeros, used during creates/deletes @@ -161,3 +161,31 @@ class GithubEventFilter(EventFilter): return False return True + + +class GithubRefFilter(RefFilter): + def __init__(self, statuses=[]): + RefFilter.__init__(self) + + self.statuses = statuses + + def __repr__(self): + ret = '