From e97566ad65d7fa8a5ca67f84ac2b40548fbb2652 Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Thu, 29 Apr 2021 10:36:56 +0200 Subject: [PATCH] Fix broken jobs overriding checkout to an unprotected branch The global repo state filters out unprotected branches if we exclude unprotected branches. However it is possible to define override-checkout to an unprotected branch. Handling override-checkout has been overlooked when creating the repo state. If that's not in the repo state the checkout will fail on the executor with an exception during job execution [1]. [1] Trace: Exception while executing job Traceback (most recent call last): File "/Users/q479080/git/zuul/zuul/.tox/py39/lib/python3.9/site-packages/git/refs/symbolic.py", line 302, in set_reference obj = self.repo.rev_parse(ref + "^{}") # optionally deref tags File "/Users/q479080/git/zuul/zuul/.tox/py39/lib/python3.9/site-packages/git/repo/fun.py", line 213, in rev_parse obj = name_to_object(repo, rev[:start]) File "/Users/q479080/git/zuul/zuul/.tox/py39/lib/python3.9/site-packages/git/repo/fun.py", line 147, in name_to_object raise BadName(name) gitdb.exc.BadName: Ref 'feat-x' did not resolve to an object The above exception was the direct cause of the following exception: Traceback (most recent call last): File "/Users/q479080/git/zuul/zuul/zuul/executor/server.py", line 947, in execute self._execute() File "/Users/q479080/git/zuul/zuul/tests/base.py", line 3008, in _execute super()._execute() File "/Users/q479080/git/zuul/zuul/zuul/executor/server.py", line 1147, in _execute repo.checkout(selected_ref) File "/Users/q479080/git/zuul/zuul/zuul/merger/merger.py", line 528, in checkout repo.head.reference = ref File "/Users/q479080/git/zuul/zuul/.tox/py39/lib/python3.9/site-packages/git/refs/symbolic.py", line 305, in set_reference raise ValueError("Could not extract object from %s" % ref) from e ValueError: Could not extract object from feat-x Change-Id: Ia0020ba29cc54edd2544693bcd11b90dee8fe46d --- .../git/common-config/zuul.yaml | 12 ++++++++ .../git/org_requiredproject-github/readme.md | 1 + .../org_requiringproject-github/.zuul.yaml | 13 +++++++++ .../playbooks/require-test.yaml | 2 ++ .../config/global-repo-state/main.yaml | 6 ++++ tests/unit/test_v3.py | 29 +++++++++++++++++++ zuul/manager/__init__.py | 23 ++++++++++++--- 7 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 tests/fixtures/config/global-repo-state/git/org_requiredproject-github/readme.md create mode 100644 tests/fixtures/config/global-repo-state/git/org_requiringproject-github/.zuul.yaml create mode 100644 tests/fixtures/config/global-repo-state/git/org_requiringproject-github/playbooks/require-test.yaml diff --git a/tests/fixtures/config/global-repo-state/git/common-config/zuul.yaml b/tests/fixtures/config/global-repo-state/git/common-config/zuul.yaml index 5e04280383..4cee2e604f 100644 --- a/tests/fixtures/config/global-repo-state/git/common-config/zuul.yaml +++ b/tests/fixtures/config/global-repo-state/git/common-config/zuul.yaml @@ -26,16 +26,28 @@ - event: comment-added approval: - Code-Review: 2 + github: + - event: pull_request + action: opened success: gerrit: Verified: 1 submit: true + github: + check: success + comment: false failure: gerrit: Verified: -1 + github: + check: failure + comment: false start: gerrit: Verified: 0 + github: + check: in_progress + comment: false precedence: high - job: diff --git a/tests/fixtures/config/global-repo-state/git/org_requiredproject-github/readme.md b/tests/fixtures/config/global-repo-state/git/org_requiredproject-github/readme.md new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/global-repo-state/git/org_requiredproject-github/readme.md @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/global-repo-state/git/org_requiringproject-github/.zuul.yaml b/tests/fixtures/config/global-repo-state/git/org_requiringproject-github/.zuul.yaml new file mode 100644 index 0000000000..1799cfa080 --- /dev/null +++ b/tests/fixtures/config/global-repo-state/git/org_requiringproject-github/.zuul.yaml @@ -0,0 +1,13 @@ +- job: + name: require-test1-github + parent: null + run: playbooks/require-test.yaml + required-projects: + - name: org/requiredproject-github + # Checkout unprotected branch + override-checkout: feat-x + +- project: + check: + jobs: + - require-test1-github diff --git a/tests/fixtures/config/global-repo-state/git/org_requiringproject-github/playbooks/require-test.yaml b/tests/fixtures/config/global-repo-state/git/org_requiringproject-github/playbooks/require-test.yaml new file mode 100644 index 0000000000..4ed83372ba --- /dev/null +++ b/tests/fixtures/config/global-repo-state/git/org_requiringproject-github/playbooks/require-test.yaml @@ -0,0 +1,2 @@ +- hosts: localhost + tasks: [] diff --git a/tests/fixtures/config/global-repo-state/main.yaml b/tests/fixtures/config/global-repo-state/main.yaml index f9c3231f57..75164643da 100644 --- a/tests/fixtures/config/global-repo-state/main.yaml +++ b/tests/fixtures/config/global-repo-state/main.yaml @@ -1,5 +1,6 @@ - tenant: name: tenant-one + exclude-unprotected-branches: true source: gerrit: config-projects: @@ -10,3 +11,8 @@ - org/requiredproject - org/requiringproject - org/roles + github: + config-projects: + - org/requiringproject-github + untrusted-projects: + - org/requiredproject-github diff --git a/tests/unit/test_v3.py b/tests/unit/test_v3.py index bf82c22f9d..6b99e33510 100644 --- a/tests/unit/test_v3.py +++ b/tests/unit/test_v3.py @@ -2436,6 +2436,7 @@ class TestInRepoConfig(ZuulTestCase): class TestGlobalRepoState(AnsibleZuulTestCase): + config_file = 'zuul-connections-gerrit-and-github.conf' tenant_config_file = 'config/global-repo-state/main.yaml' def test_inherited_playbooks(self): @@ -2484,6 +2485,34 @@ class TestGlobalRepoState(AnsibleZuulTestCase): dict(name='test2', result='SUCCESS', changes='1,1'), ]) + def test_required_projects_unprotected_override_checkout(self): + # Setup branch protection for master on org/requiringproject-github + github = self.fake_github.getGithubClient() + github.repo_from_project( + 'org/requiringproject-github')._set_branch_protection( + 'master', True) + + # Create unprotected branch feat-x. This branch will be the target + # of override-checkout + repo = github.repo_from_project('org/requiredproject-github') + repo._set_branch_protection('master', True) + repo._create_branch('feat-x') + self.create_branch('org/requiredproject-github', 'feat-x') + + # Reconfigure to ensure zuul knows about the branch protection + self.scheds.execute(lambda app: app.sched.reconfigure(app.config)) + self.waitUntilSettled() + + A = self.fake_github.openFakePullRequest( + 'org/requiringproject-github', 'master', 'A') + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + # Job must be successful + self.assertHistory([ + dict(name='require-test1-github', result='SUCCESS'), + ]) + def test_required_projects(self): # Test that the repo state is restored globally for the whole buildset # including required projects not in the dependency chain. diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index 979ce317f6..e84437428d 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -864,6 +864,7 @@ class PipelineManager(metaclass=ABCMeta): return self._loadDynamicLayout(item) def _branchesForRepoState(self, projects, tenant, items=None): + items = items or [] if all(tenant.getExcludeUnprotectedBranches(project) for project in projects): branches = set() @@ -873,9 +874,22 @@ class PipelineManager(metaclass=ABCMeta): branches.update(tenant.getProjectBranches(project)) # Additionally add all target branches of all involved items. - if items is not None: - branches.update(item.change.branch for item in items - if hasattr(item.change, 'branch')) + branches.update(item.change.branch for item in items + if hasattr(item.change, 'branch')) + + # Make sure override-checkout targets are part of the repo state + for item in items: + if not item.job_graph: + continue + + for job in item.job_graph.jobs.values(): + if job.override_checkout: + branches.add(job.override_checkout) + + for p in job.required_projects.values(): + if p.override_checkout: + branches.add(p.override_checkout) + branches = list(branches) else: branches = None @@ -959,7 +973,8 @@ class PipelineManager(metaclass=ABCMeta): item.current_build_set.COMPLETE return True - branches = self._branchesForRepoState(projects=projects, tenant=tenant) + branches = self._branchesForRepoState( + projects=projects, tenant=tenant, items=[item]) new_items = list() for project in projects: