Don't reconfigure tenant on unprotected branch pushes
We currently do a tenant reconfiguration on any branch push containing zuul config changes. However in the GitHub usecase unprotected branches are often excluded from the configuration. When pushing to unprotected branches (which is the majority in our deployment) the tenant reconfiguration is useless and can be skipped. Change-Id: I0b73b3472b4a575630cf277e32a64ffbb086fe80
This commit is contained in:
Tobias Henkel
parent
306039f2dc
commit
a784a4fbb7
|
|
@ -31,6 +31,12 @@ class FakeBranch(object):
|
|||
self.name = branch
|
||||
self.protected = protected
|
||||
|
||||
def as_dict(self):
|
||||
return {
|
||||
'name': self.name,
|
||||
'protected': self.protected
|
||||
}
|
||||
|
||||
|
||||
class FakeStatus(object):
|
||||
def __init__(self, state, url, description, context, user):
|
||||
|
|
@ -119,20 +125,31 @@ class FakeRepository(object):
|
|||
entity, request = path.split('/', 1)
|
||||
|
||||
if entity == 'branches':
|
||||
return self.get_url_branch(request)
|
||||
return self.get_url_branches(request)
|
||||
if entity == 'collaborators':
|
||||
return self.get_url_collaborators(request)
|
||||
else:
|
||||
return None
|
||||
|
||||
def get_url_branch(self, path):
|
||||
branch, entity = path.split('/')
|
||||
def get_url_branches(self, path):
|
||||
elements = path.split('/')
|
||||
|
||||
branch = elements[0]
|
||||
if len(elements) == 1:
|
||||
return self.get_url_branch(branch)
|
||||
|
||||
entity = elements[1]
|
||||
if entity == 'protection':
|
||||
return self.get_url_protection(branch)
|
||||
else:
|
||||
return None
|
||||
|
||||
def get_url_branch(self, branch_name):
|
||||
for branch in self._branches:
|
||||
if branch.name == branch_name:
|
||||
return FakeResponse(branch.as_dict())
|
||||
return FakeResponse(None, 404)
|
||||
|
||||
def get_url_collaborators(self, path):
|
||||
login, entity = path.split('/')
|
||||
|
||||
|
|
@ -257,8 +274,8 @@ class FakeIssueSearchResult(object):
|
|||
|
||||
|
||||
class FakeResponse(object):
|
||||
def __init__(self, data):
|
||||
self.status_code = 200
|
||||
def __init__(self, data, status_code=200):
|
||||
self.status_code = status_code
|
||||
self.data = data
|
||||
|
||||
def json(self):
|
||||
|
|
|
|||
|
|
@ -844,6 +844,79 @@ class TestGithubUnprotectedBranches(ZuulTestCase):
|
|||
self.assertIn('master', tpc1.parsed_branch_config.keys())
|
||||
self.assertIn('master', tpc2.parsed_branch_config.keys())
|
||||
|
||||
def test_unprotected_push(self):
|
||||
"""Test that unprotected pushes don't cause tenant reconfigurations"""
|
||||
|
||||
# Prepare repo with an initial commit
|
||||
A = self.fake_github.openFakePullRequest('org/project2', 'master', 'A')
|
||||
A.setMerged("merging A")
|
||||
|
||||
# Do a push on top of A
|
||||
pevent = self.fake_github.getPushEvent(project='org/project2',
|
||||
old_rev=A.head_sha,
|
||||
ref='refs/heads/master',
|
||||
modified_files=['zuul.yaml'])
|
||||
|
||||
# record previous tenant reconfiguration time, which may not be set
|
||||
old = self.sched.tenant_last_reconfigured.get('tenant-one', 0)
|
||||
self.waitUntilSettled()
|
||||
time.sleep(1)
|
||||
|
||||
self.fake_github.emitEvent(pevent)
|
||||
self.waitUntilSettled()
|
||||
new = self.sched.tenant_last_reconfigured.get('tenant-one', 0)
|
||||
|
||||
# We don't expect a reconfiguration because the push was to an
|
||||
# unprotected branch
|
||||
self.assertEqual(old, new)
|
||||
|
||||
# now enable branch protection and trigger the push event again
|
||||
github = self.fake_github.getGithubClient()
|
||||
repo = github.repo_from_project('org/project2')
|
||||
repo._set_branch_protection('master', True)
|
||||
|
||||
self.fake_github.emitEvent(pevent)
|
||||
self.waitUntilSettled()
|
||||
new = self.sched.tenant_last_reconfigured.get('tenant-one', 0)
|
||||
|
||||
# We now expect that zuul reconfigured itself
|
||||
self.assertLess(old, new)
|
||||
|
||||
def test_unprotected_branch_delete(self):
|
||||
"""Test that protected branch deletes trigger a tenant reconfig"""
|
||||
|
||||
# Prepare repo with an initial commit
|
||||
A = self.fake_github.openFakePullRequest('org/project2', 'master', 'A')
|
||||
A.setMerged("merging A")
|
||||
|
||||
# now enable branch protection and trigger a reconfiguration
|
||||
github = self.fake_github.getGithubClient()
|
||||
repo = github.repo_from_project('org/project2')
|
||||
repo._set_branch_protection('feat-x', True)
|
||||
|
||||
self.sched.reconfigure(self.config)
|
||||
self.waitUntilSettled()
|
||||
|
||||
# record previous tenant reconfiguration time, which may not be set
|
||||
old = self.sched.tenant_last_reconfigured.get('tenant-one', 0)
|
||||
self.waitUntilSettled()
|
||||
time.sleep(1)
|
||||
|
||||
# Delete the branch
|
||||
pevent = self.fake_github.getPushEvent(project='org/project2',
|
||||
old_rev=A.head_sha,
|
||||
new_rev='0' * 40,
|
||||
ref='refs/heads/master',
|
||||
modified_files=['zuul.yaml'])
|
||||
|
||||
self.fake_github.emitEvent(pevent)
|
||||
self.waitUntilSettled()
|
||||
new = self.sched.tenant_last_reconfigured.get('tenant-one', 0)
|
||||
|
||||
# We now expect that zuul reconfigured itself as we deleted a protected
|
||||
# branch
|
||||
self.assertLess(old, new)
|
||||
|
||||
|
||||
class TestGithubWebhook(ZuulTestCase):
|
||||
config_file = 'zuul-github-driver.conf'
|
||||
|
|
|
|||
|
|
@ -256,6 +256,26 @@ class GithubEventConnector(threading.Thread):
|
|||
if event.newrev == '0' * 40:
|
||||
event.branch_deleted = True
|
||||
|
||||
if event.branch:
|
||||
if event.branch_deleted:
|
||||
# We currently cannot determine if a deleted branch was
|
||||
# protected so we need to assume it was. GitHub doesn't allow
|
||||
# deletion of protected branches but we don't get a
|
||||
# notification about branch protection settings. Thus we don't
|
||||
# know if branch protection has been disabled before deletion
|
||||
# of the branch.
|
||||
# FIXME(tobiash): Find a way to handle that case
|
||||
event.branch_protected = True
|
||||
elif event.branch_created:
|
||||
# A new branch never can be protected because that needs to be
|
||||
# configured after it has been created.
|
||||
event.branch_protected = False
|
||||
else:
|
||||
# An updated branch can be protected or not so we have to ask
|
||||
# GitHub whether it is.
|
||||
b = self.connection.getBranch(event.project_name, event.branch)
|
||||
event.branch_protected = b.get('protected')
|
||||
|
||||
return event
|
||||
|
||||
def _event_pull_request(self, body):
|
||||
|
|
@ -910,6 +930,22 @@ class GithubConnection(BaseConnection):
|
|||
|
||||
return self._project_branch_cache[project.name]
|
||||
|
||||
def getBranch(self, project_name, branch):
|
||||
github = self.getGithubClient(project_name)
|
||||
|
||||
# Note that we directly use a web request here because if we use the
|
||||
# github3.py api directly we need a repository object which needs
|
||||
# an unneeded web request during creation.
|
||||
url = github.session.build_url('repos', project_name,
|
||||
'branches', branch)
|
||||
|
||||
resp = github.session.get(url)
|
||||
|
||||
if resp.status_code == 404:
|
||||
return None
|
||||
|
||||
return resp.json()
|
||||
|
||||
def getPullUrl(self, project, number):
|
||||
return '%s/pull/%s' % (self.getGitwebUrl(project), number)
|
||||
|
||||
|
|
|
|||
|
|
@ -2431,6 +2431,7 @@ class TriggerEvent(object):
|
|||
self.branch_updated = False
|
||||
self.branch_created = False
|
||||
self.branch_deleted = False
|
||||
self.branch_protected = True
|
||||
self.ref = None
|
||||
# For management events (eg: enqueue / promote)
|
||||
self.tenant_name = None
|
||||
|
|
|
|||
|
|
@ -934,11 +934,24 @@ class Scheduler(threading.Thread):
|
|||
"source %s",
|
||||
e.change, project.source)
|
||||
continue
|
||||
reconfigure_tenant = False
|
||||
if ((event.branch_updated and
|
||||
hasattr(change, 'files') and
|
||||
change.updatesConfig()) or
|
||||
event.branch_created or
|
||||
event.branch_deleted):
|
||||
|
||||
reconfigure_tenant = True
|
||||
|
||||
# If the branch is unprotected and unprotected branches
|
||||
# are excluded from the tenant for that project skip reconfig.
|
||||
if (reconfigure_tenant and not
|
||||
event.branch_protected and
|
||||
tenant.getExcludeUnprotectedBranches(project)):
|
||||
|
||||
reconfigure_tenant = False
|
||||
|
||||
if reconfigure_tenant:
|
||||
# The change that just landed updates the config
|
||||
# or a branch was just created or deleted. Clear
|
||||
# out cached data for this project and perform a
|
||||
|
|
|
|||
Loading…
Reference in New Issue