Handle change related reqs on push like events
Push and ref-updated events do not have an associated change or pull-request. Thus, requirements that are specific to changes or pull-requests cannot possibly be met. To make this work, Ref filters should only be applied to changes that come from a matching connection. This was not previously enforced when filters became connection specific. Change-Id: I5d67bef264db0ad5ba3a7180ce74e3670ba822ce Story: 2000774 Task: 4624
This commit is contained in:
parent
6a22df5f54
commit
8c2eb573be
|
@ -592,6 +592,21 @@ class FakeGithubPullRequest(object):
|
|||
def getPullRequestClosedEvent(self):
|
||||
return self._getPullRequestEvent('closed')
|
||||
|
||||
def getPushEvent(self, old_sha, ref='refs/heads/master'):
|
||||
name = 'push'
|
||||
data = {
|
||||
'ref': ref,
|
||||
'before': old_sha,
|
||||
'after': self.head_sha,
|
||||
'repository': {
|
||||
'full_name': self.project
|
||||
},
|
||||
'sender': {
|
||||
'login': 'ghuser'
|
||||
}
|
||||
}
|
||||
return (name, data)
|
||||
|
||||
def addComment(self, message):
|
||||
self.comments.append(message)
|
||||
self._updateTimeStamp()
|
||||
|
|
|
@ -0,0 +1,2 @@
|
|||
- hosts: all
|
||||
tasks: []
|
|
@ -0,0 +1,119 @@
|
|||
- pipeline:
|
||||
name: current
|
||||
manager: independent
|
||||
require:
|
||||
github:
|
||||
current-patchset: true
|
||||
gerrit:
|
||||
current-patchset: true
|
||||
trigger:
|
||||
github:
|
||||
- event: push
|
||||
gerrit:
|
||||
- event: ref-updated
|
||||
|
||||
- pipeline:
|
||||
name: open
|
||||
manager: independent
|
||||
require:
|
||||
github:
|
||||
open: true
|
||||
gerrit:
|
||||
open: true
|
||||
trigger:
|
||||
github:
|
||||
- event: push
|
||||
gerrit:
|
||||
- event: ref-updated
|
||||
|
||||
- pipeline:
|
||||
name: review
|
||||
manager: independent
|
||||
require:
|
||||
github:
|
||||
review:
|
||||
- type: approval
|
||||
gerrit:
|
||||
approval:
|
||||
- email: herp@derp.invalid
|
||||
trigger:
|
||||
github:
|
||||
- event: push
|
||||
gerrit:
|
||||
- event: ref-updated
|
||||
|
||||
- pipeline:
|
||||
name: status
|
||||
manager: independent
|
||||
require:
|
||||
github:
|
||||
status: 'zuul:check:success'
|
||||
trigger:
|
||||
github:
|
||||
- event: push
|
||||
|
||||
- pipeline:
|
||||
name: pushhub
|
||||
manager: independent
|
||||
require:
|
||||
gerrit:
|
||||
open: true
|
||||
trigger:
|
||||
github:
|
||||
- event: push
|
||||
gerrit:
|
||||
- event: ref-updated
|
||||
|
||||
- pipeline:
|
||||
name: pushgerrit
|
||||
manager: independent
|
||||
require:
|
||||
github:
|
||||
open: true
|
||||
trigger:
|
||||
github:
|
||||
- event: push
|
||||
gerrit:
|
||||
- event: ref-updated
|
||||
|
||||
- job:
|
||||
name: job1
|
||||
|
||||
- project:
|
||||
name: org/project1
|
||||
current:
|
||||
jobs:
|
||||
- job1
|
||||
open:
|
||||
jobs:
|
||||
- job1
|
||||
review:
|
||||
jobs:
|
||||
- job1
|
||||
status:
|
||||
jobs:
|
||||
- job1
|
||||
pushhub:
|
||||
jobs:
|
||||
- job1
|
||||
pushgerrit:
|
||||
jobs:
|
||||
- job1
|
||||
|
||||
- project:
|
||||
name: org/project2
|
||||
current:
|
||||
jobs:
|
||||
- job1
|
||||
open:
|
||||
jobs:
|
||||
- job1
|
||||
review:
|
||||
jobs:
|
||||
- job1
|
||||
pushhub:
|
||||
jobs:
|
||||
- job1
|
||||
pushgerrit:
|
||||
jobs:
|
||||
- job1
|
|
@ -0,0 +1 @@
|
|||
test
|
|
@ -0,0 +1 @@
|
|||
test
|
|
@ -0,0 +1,11 @@
|
|||
- tenant:
|
||||
name: tenant-one
|
||||
source:
|
||||
github:
|
||||
config-projects:
|
||||
- common-config
|
||||
untrusted-projects:
|
||||
- org/project1
|
||||
gerrit:
|
||||
untrusted-projects:
|
||||
- org/project2
|
|
@ -0,0 +1,23 @@
|
|||
[gearman]
|
||||
server=127.0.0.1
|
||||
|
||||
[zuul]
|
||||
job_name_in_report=true
|
||||
status_url=http://zuul.example.com/status
|
||||
|
||||
[merger]
|
||||
git_dir=/tmp/zuul-test/git
|
||||
git_user_email=zuul@example.com
|
||||
git_user_name=zuul
|
||||
zuul_url=http://zuul.example.com/p
|
||||
|
||||
[executor]
|
||||
git_dir=/tmp/zuul-test/executor-git
|
||||
|
||||
[connection github]
|
||||
driver=github
|
||||
|
||||
[connection gerrit]
|
||||
driver=gerrit
|
||||
server=review.example.com
|
||||
user=jenkins
|
|
@ -0,0 +1,53 @@
|
|||
# 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
|
||||
|
||||
|
||||
class TestPushRequirements(ZuulTestCase):
|
||||
config_file = 'zuul-push-reqs.conf'
|
||||
tenant_config_file = 'config/push-reqs/main.yaml'
|
||||
|
||||
def setup_config(self):
|
||||
super(TestPushRequirements, self).setup_config()
|
||||
|
||||
def test_push_requirements(self):
|
||||
self.executor_server.hold_jobs_in_build = True
|
||||
|
||||
# Create a github change, add a change and emit a push event
|
||||
A = self.fake_github.openFakePullRequest('org/project1', 'master', 'A')
|
||||
old_sha = A.head_sha
|
||||
self.fake_github.emitEvent(A.getPushEvent(old_sha))
|
||||
|
||||
self.waitUntilSettled()
|
||||
|
||||
# All but one pipeline should be skipped
|
||||
self.assertEqual(1, len(self.builds))
|
||||
self.assertEqual('pushhub', self.builds[0].pipeline)
|
||||
self.assertEqual('org/project1', self.builds[0].project)
|
||||
|
||||
# Make a gerrit change, and emit a ref-updated event
|
||||
B = self.fake_gerrit.addFakeChange('org/project2', 'master', 'B')
|
||||
self.fake_gerrit.addEvent(B.getRefUpdatedEvent())
|
||||
|
||||
self.waitUntilSettled()
|
||||
|
||||
# All but one pipeline should be skipped, increasing builds by 1
|
||||
self.assertEqual(2, len(self.builds))
|
||||
self.assertEqual('pushgerrit', self.builds[1].pipeline)
|
||||
self.assertEqual('org/project2', self.builds[1].project)
|
||||
|
||||
self.executor_server.hold_jobs_in_build = False
|
||||
self.executor_server.release()
|
||||
self.waitUntilSettled()
|
|
@ -115,6 +115,10 @@ class GerritApprovalFilter(object):
|
|||
return True
|
||||
|
||||
def matchesApprovals(self, change):
|
||||
if self.required_approvals or self.reject_approvals:
|
||||
if not hasattr(change, 'number'):
|
||||
# Not a change, no reviews
|
||||
return False
|
||||
if (self.required_approvals and not change.approvals
|
||||
or self.reject_approvals and not change.approvals):
|
||||
# A change with no approvals can not match
|
||||
|
@ -291,10 +295,10 @@ class GerritEventFilter(EventFilter, GerritApprovalFilter):
|
|||
|
||||
|
||||
class GerritRefFilter(RefFilter, GerritApprovalFilter):
|
||||
def __init__(self, open=None, current_patchset=None,
|
||||
def __init__(self, connection_name, open=None, current_patchset=None,
|
||||
statuses=[], required_approvals=[],
|
||||
reject_approvals=[]):
|
||||
RefFilter.__init__(self)
|
||||
RefFilter.__init__(self, connection_name)
|
||||
|
||||
GerritApprovalFilter.__init__(self,
|
||||
required_approvals=required_approvals,
|
||||
|
@ -307,6 +311,7 @@ class GerritRefFilter(RefFilter, GerritApprovalFilter):
|
|||
def __repr__(self):
|
||||
ret = '<GerritRefFilter'
|
||||
|
||||
ret += ' connection_name: %s' % self.connection_name
|
||||
if self.open is not None:
|
||||
ret += ' open: %s' % self.open
|
||||
if self.current_patchset is not None:
|
||||
|
@ -325,11 +330,21 @@ class GerritRefFilter(RefFilter, GerritApprovalFilter):
|
|||
|
||||
def matches(self, change):
|
||||
if self.open is not None:
|
||||
if self.open != change.open:
|
||||
# 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.open != change.open:
|
||||
return False
|
||||
else:
|
||||
return False
|
||||
|
||||
if self.current_patchset is not None:
|
||||
if self.current_patchset != change.is_current_patchset:
|
||||
# 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.current_patchset != change.is_current_patchset:
|
||||
return False
|
||||
else:
|
||||
return False
|
||||
|
||||
if self.statuses:
|
||||
|
|
|
@ -65,6 +65,7 @@ class GerritSource(BaseSource):
|
|||
|
||||
def getRequireFilters(self, config):
|
||||
f = GerritRefFilter(
|
||||
connection_name=self.connection.connection_name,
|
||||
open=config.get('open'),
|
||||
current_patchset=config.get('current-patchset'),
|
||||
statuses=to_list(config.get('status')),
|
||||
|
@ -74,6 +75,7 @@ class GerritSource(BaseSource):
|
|||
|
||||
def getRejectFilters(self, config):
|
||||
f = GerritRefFilter(
|
||||
connection_name=self.connection.connection_name,
|
||||
reject_approvals=to_list(config.get('approval')),
|
||||
)
|
||||
return [f]
|
||||
|
|
|
@ -109,9 +109,13 @@ class GithubCommonFilter(object):
|
|||
return True
|
||||
|
||||
def matchesReviews(self, change):
|
||||
if self.required_reviews and not change.reviews:
|
||||
# No reviews means no matching
|
||||
return False
|
||||
if self.required_reviews:
|
||||
if not hasattr(change, 'number'):
|
||||
# not a PR, no reviews
|
||||
return False
|
||||
if not change.reviews:
|
||||
# No reviews means no matching
|
||||
return False
|
||||
|
||||
return self.matchesRequiredReviews(change)
|
||||
|
||||
|
@ -133,6 +137,9 @@ class GithubCommonFilter(object):
|
|||
# statuses and the filter statuses are a null intersection, there
|
||||
# are no matches and we return false
|
||||
if self.required_statuses:
|
||||
if not hasattr(change, 'number'):
|
||||
# not a PR, no status
|
||||
return False
|
||||
if set(change.status).isdisjoint(set(self.required_statuses)):
|
||||
return False
|
||||
return True
|
||||
|
@ -263,9 +270,9 @@ class GithubEventFilter(EventFilter, GithubCommonFilter):
|
|||
|
||||
|
||||
class GithubRefFilter(RefFilter, GithubCommonFilter):
|
||||
def __init__(self, statuses=[], required_reviews=[], open=None,
|
||||
current_patchset=None):
|
||||
RefFilter.__init__(self)
|
||||
def __init__(self, connection_name, statuses=[], required_reviews=[],
|
||||
open=None, current_patchset=None):
|
||||
RefFilter.__init__(self, connection_name)
|
||||
|
||||
GithubCommonFilter.__init__(self, required_reviews=required_reviews,
|
||||
required_statuses=statuses)
|
||||
|
@ -276,6 +283,7 @@ class GithubRefFilter(RefFilter, GithubCommonFilter):
|
|||
def __repr__(self):
|
||||
ret = '<GithubRefFilter'
|
||||
|
||||
ret += ' connection_name: %s' % self.connection_name
|
||||
if self.statuses:
|
||||
ret += ' statuses: %s' % ', '.join(self.statuses)
|
||||
if self.required_reviews:
|
||||
|
@ -295,11 +303,21 @@ class GithubRefFilter(RefFilter, GithubCommonFilter):
|
|||
return False
|
||||
|
||||
if self.open is not None:
|
||||
if self.open != change.open:
|
||||
# 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.open != change.open:
|
||||
return False
|
||||
else:
|
||||
return False
|
||||
|
||||
if self.current_patchset is not None:
|
||||
if self.current_patchset != change.is_current_patchset:
|
||||
# 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.current_patchset != change.is_current_patchset:
|
||||
return False
|
||||
else:
|
||||
return False
|
||||
|
||||
# required reviews are ANDed
|
||||
|
|
|
@ -96,6 +96,7 @@ class GithubSource(BaseSource):
|
|||
|
||||
def getRequireFilters(self, config):
|
||||
f = GithubRefFilter(
|
||||
connection_name=self.connection.connection_name,
|
||||
statuses=to_list(config.get('status')),
|
||||
required_reviews=to_list(config.get('review')),
|
||||
open=config.get('open'),
|
||||
|
|
|
@ -285,6 +285,10 @@ class PipelineManager(object):
|
|||
|
||||
if not ignore_requirements:
|
||||
for f in self.changeish_filters:
|
||||
if f.connection_name != change.project.connection_name:
|
||||
self.log.debug("Filter %s skipped for change %s due "
|
||||
"to mismatched connections" % (f, change))
|
||||
continue
|
||||
if not f.matches(change):
|
||||
self.log.debug("Change %s does not match pipeline "
|
||||
"requirement %s" % (change, f))
|
||||
|
|
|
@ -1924,8 +1924,9 @@ class EventFilter(BaseFilter):
|
|||
|
||||
class RefFilter(BaseFilter):
|
||||
"""Allows a Manager to only enqueue Changes that meet certain criteria."""
|
||||
def __init__(self):
|
||||
def __init__(self, connection_name):
|
||||
super(RefFilter, self).__init__()
|
||||
self.connection_name = connection_name
|
||||
|
||||
def matches(self, change):
|
||||
return True
|
||||
|
|
Loading…
Reference in New Issue