From 2a6d129ea1f0981b7635f60813e9735b21b97903 Mon Sep 17 00:00:00 2001 From: Markus Hosch Date: Wed, 5 Sep 2018 16:01:59 +0200 Subject: [PATCH] Reload tenant in case of new project branches Since GitHub doesn't report branch protection changes, it is possible that a pull request to a branch is not considered since the configuration for that branch isn't loaded yet. This is, however, surprising for the user. This change updates the branch protection cache if a pull request was received and when the trigger event is then processed in the scheduler, it checks if the target branch is a protected branch whose config is not yet in the tenant's configuration cache. If that's the case, a tenant reload is issued. Change-Id: I0d851a8773b73de54b0b777691a6a0d9f788f096 --- .../git/org_project2/playbooks/used-job.yaml | 2 + .../git/org_project2/zuul.yaml | 8 +++- tests/unit/test_github_driver.py | 40 +++++++++++++++++++ zuul/driver/github/githubconnection.py | 29 +++++++++----- zuul/scheduler.py | 8 ++++ 5 files changed, 76 insertions(+), 11 deletions(-) create mode 100644 tests/fixtures/config/unprotected-branches/git/org_project2/playbooks/used-job.yaml diff --git a/tests/fixtures/config/unprotected-branches/git/org_project2/playbooks/used-job.yaml b/tests/fixtures/config/unprotected-branches/git/org_project2/playbooks/used-job.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/git/org_project2/playbooks/used-job.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml b/tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml index 61c38d7836..0d11c41d9b 100644 --- a/tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml +++ b/tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml @@ -1,2 +1,8 @@ - job: - name: unused-job + name: used-job + run: playbooks/used-job.yaml + +- project: + check: + jobs: + - used-job diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index fb807a9947..29845a3b21 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -1004,6 +1004,7 @@ class TestGithubUnprotectedBranches(ZuulTestCase): time.sleep(1) # Delete the branch + repo._delete_branch('master') pevent = self.fake_github.getPushEvent(project='org/project2', old_rev=A.head_sha, new_rev='0' * 40, @@ -1018,6 +1019,45 @@ class TestGithubUnprotectedBranches(ZuulTestCase): # branch self.assertLess(old, new) + # This test verifies that a PR is considered in case it was created for + # a branch just has been set to protected before a tenant reconfiguration + # took place. + def test_reconfigure_on_pr_to_new_protected_branch(self): + self.create_branch('org/project2', 'release') + + github = self.fake_github.getGithubClient() + repo = github.repo_from_project('org/project2') + repo._set_branch_protection('master', True) + repo._create_branch('release') + repo._create_branch('feature') + + self.sched.reconfigure(self.config) + self.waitUntilSettled() + + repo._set_branch_protection('release', True) + + self.executor_server.hold_jobs_in_build = True + + A = self.fake_github.openFakePullRequest( + 'org/project2', 'release', 'A') + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + self.executor_server.hold_jobs_in_build = False + self.executor_server.release() + self.waitUntilSettled() + + self.assertEqual('SUCCESS', + self.getJobFromHistory('used-job').result) + + job = self.getJobFromHistory('used-job') + zuulvars = job.parameters['zuul'] + self.assertEqual(str(A.number), zuulvars['change']) + self.assertEqual(str(A.head_sha), zuulvars['patchset']) + self.assertEqual('release', zuulvars['branch']) + + self.assertEqual(1, len(self.history)) + class TestGithubWebhook(ZuulTestCase): config_file = 'zuul-github-driver.conf' diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 81a35560b4..3d37a604e8 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -220,12 +220,30 @@ class GithubEventConnector(threading.Thread): if event: event.delivery = delivery + project = self.connection.source.getProject(event.project_name) if event.change_number: - project = self.connection.source.getProject(event.project_name) self.connection._getChange(project, event.change_number, event.patch_number, refresh=True) + + # 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: + b = self.connection.getBranch(project.name, event.branch) + if b is not None: + branch_protected = b.get('protected') + self.connection.checkBranchCache(project, event.branch, + branch_protected) + event.branch_protected = branch_protected + else: + # This can happen if the branch was deleted in GitHub. In + # this case we assume that the branch COULD have been + # protected before. The cache update is handled by the + # push event, so we don't touch the cache here again. + event.branch_protected = True + event.project_hostname = self.connection.canonical_hostname self.connection.logEvent(event) self.connection.sched.addEvent(event) @@ -268,20 +286,11 @@ class GithubEventConnector(threading.Thread): # 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 self.connection._clearBranchCache(project) 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 self.connection._clearBranchCache(project) - 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') - self.connection.checkBranchCache(project, event.branch, - event.branch_protected) return event diff --git a/zuul/scheduler.py b/zuul/scheduler.py index e16e30a8e2..7dbd82ecf3 100644 --- a/zuul/scheduler.py +++ b/zuul/scheduler.py @@ -1032,7 +1032,15 @@ class Scheduler(threading.Thread): (event.branch_deleted and self.abide.getUnparsedConfig(event.project_name, event.branch) is not None)): + reconfigure_tenant = True + # If the driver knows the branch but we don't have a config, we + # also need to reconfigure. This happens if a GitHub branch + # was just configured as protected without a push in between. + if (event.branch in project.source.getProjectBranches( + project, tenant) + and self.abide.getUnparsedConfig( + project.canonical_name, event.branch) is None): reconfigure_tenant = True # If the branch is unprotected and unprotected branches