From 735190f2ec538d8e19d9db707d827cb6c84bf901 Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Tue, 15 May 2018 09:33:46 +0200 Subject: [PATCH] Support merged as requirement in github driver Currently when creating a post pipeline with the github driver we can only use the push event. This has the drawback that it ignores protected branches and thus also works speculatively on any branch if you want to have a generic post pipeline. This imposes problems with secrets. So instead using the push event we want to trigger on pull_request closed events. However this currently doesn't work as we cannot distinguish between merged and abandoned pull requests. Adding a merged requirement nicely solves that problem. Change-Id: I46670c6aa036976c430a6034a6b1da0e23fa9f92 --- doc/source/admin/drivers/github.rst | 5 ++++ .../require-merged-70784e1e45cac08e.yaml | 5 ++++ tests/fixtures/layouts/basic-github.yaml | 23 +++++++++++++++++++ tests/unit/test_github_driver.py | 16 +++++++++++++ zuul/driver/github/githubmodel.py | 22 +++++++++++++++--- zuul/driver/github/githubsource.py | 4 ++++ 6 files changed, 72 insertions(+), 3 deletions(-) create mode 100644 releasenotes/notes/require-merged-70784e1e45cac08e.yaml diff --git a/doc/source/admin/drivers/github.rst b/doc/source/admin/drivers/github.rst index 9ed8b0ddc5..2f2abc9c32 100644 --- a/doc/source/admin/drivers/github.rst +++ b/doc/source/admin/drivers/github.rst @@ -409,6 +409,11 @@ enqueued into the pipeline. A boolean value (``true`` or ``false``) that indicates whether the change must be open or closed in order to be enqueued. + .. attr:: merged + + A boolean value (``true`` or ``false``) that indicates whether + the change must be merged or not in order to be enqueued. + .. attr:: current-patchset A boolean value (``true`` or ``false``) that indicates whether diff --git a/releasenotes/notes/require-merged-70784e1e45cac08e.yaml b/releasenotes/notes/require-merged-70784e1e45cac08e.yaml new file mode 100644 index 0000000000..5b41eae53f --- /dev/null +++ b/releasenotes/notes/require-merged-70784e1e45cac08e.yaml @@ -0,0 +1,5 @@ +--- +features: + - | + The GitHub driver now supports the :attr:`pipeline.require..merged` + requirement. diff --git a/tests/fixtures/layouts/basic-github.yaml b/tests/fixtures/layouts/basic-github.yaml index 217e874b29..1ab54409cb 100644 --- a/tests/fixtures/layouts/basic-github.yaml +++ b/tests/fixtures/layouts/basic-github.yaml @@ -17,6 +17,22 @@ failure: github: {} +- pipeline: + name: post + manager: independent + require: + github: + merged: true + trigger: + github: + - event: pull_request + action: + - closed + success: + github: {} + failure: + github: {} + - job: name: base parent: null @@ -30,9 +46,16 @@ name: project-test2 run: playbooks/project-test2.yaml +- job: + name: project-post + run: playbooks/project-post.yaml + - project: name: org/project check: jobs: - project-test1 - project-test2 + post: + jobs: + - project-post diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index f9e98f916c..8e87012b59 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -72,6 +72,22 @@ class TestGithubDriver(ZuulTestCase): self.assertEqual(2, len(self.history)) + # now emit closed event without merging + self.fake_github.emitEvent(A.getPullRequestClosedEvent()) + self.waitUntilSettled() + + # nothing should have happened due to the merged requirement + self.assertEqual(2, len(self.history)) + + # now merge the PR and emit the event again + A.setMerged('merged') + + self.fake_github.emitEvent(A.getPullRequestClosedEvent()) + self.waitUntilSettled() + + # post job must be run + self.assertEqual(3, len(self.history)) + @simple_layout('layouts/files-github.yaml', driver='github') def test_pull_matched_file_event(self): A = self.fake_github.openFakePullRequest( diff --git a/zuul/driver/github/githubmodel.py b/zuul/driver/github/githubmodel.py index 5a2b18bb33..66580496e7 100644 --- a/zuul/driver/github/githubmodel.py +++ b/zuul/driver/github/githubmodel.py @@ -321,9 +321,10 @@ class GithubEventFilter(EventFilter, GithubCommonFilter): class GithubRefFilter(RefFilter, GithubCommonFilter): def __init__(self, connection_name, statuses=[], required_reviews=[], - reject_reviews=[], open=None, current_patchset=None, - reject_open=None, reject_current_patchset=None, - labels=[], reject_labels=[], reject_statuses=[]): + reject_reviews=[], open=None, merged=None, + current_patchset=None, reject_open=None, reject_merged=None, + reject_current_patchset=None, labels=[], reject_labels=[], + reject_statuses=[]): RefFilter.__init__(self, connection_name) GithubCommonFilter.__init__(self, required_reviews=required_reviews, @@ -335,6 +336,10 @@ class GithubRefFilter(RefFilter, GithubCommonFilter): self.open = not reject_open else: self.open = open + if reject_merged is not None: + self.merged = not reject_merged + else: + self.merged = merged if reject_current_patchset is not None: self.current_patchset = not reject_current_patchset else: @@ -358,6 +363,8 @@ class GithubRefFilter(RefFilter, GithubCommonFilter): str(self.reject_reviews)) if self.open: ret += ' open: %s' % self.open + if self.merged: + ret += ' merged: %s' % self.merged if self.current_patchset: ret += ' current-patchset: %s' % self.current_patchset if self.labels: @@ -382,6 +389,15 @@ class GithubRefFilter(RefFilter, GithubCommonFilter): else: return False + if self.merged is not None: + # if a "change" has no number, it's not a change, but a push + # and cannot possibly pass this test. + if hasattr(change, 'number'): + if self.merged != change.is_merged: + return False + else: + return False + if self.current_patchset is not None: # if a "change" has no number, it's not a change, but a push # and cannot possibly pass this test. diff --git a/zuul/driver/github/githubsource.py b/zuul/driver/github/githubsource.py index 6d9c29c535..dbcc3cfda0 100644 --- a/zuul/driver/github/githubsource.py +++ b/zuul/driver/github/githubsource.py @@ -132,6 +132,7 @@ class GithubSource(BaseSource): statuses=to_list(config.get('status')), required_reviews=to_list(config.get('review')), open=config.get('open'), + merged=config.get('merged'), current_patchset=config.get('current-patchset'), labels=to_list(config.get('label')), ) @@ -144,6 +145,7 @@ class GithubSource(BaseSource): reject_labels=to_list(config.get('label')), reject_statuses=to_list(config.get('status')), reject_open=config.get('open'), + reject_merged=config.get('merged'), reject_current_patchset=config.get('current-patchset'), ) return [f] @@ -165,6 +167,7 @@ def getRequireSchema(): require = {'status': scalar_or_list(str), 'review': scalar_or_list(review), 'open': bool, + 'merged': bool, 'current-patchset': bool, 'label': scalar_or_list(str)} return require @@ -174,6 +177,7 @@ def getRejectSchema(): reject = {'status': scalar_or_list(str), 'review': scalar_or_list(review), 'open': bool, + 'merged': bool, 'current-patchset': bool, 'label': scalar_or_list(str)} return reject