zuul
/
zuul
Code Issues Proposed changes

Report a config error for unsupported merge mode 

This updates the branch cache (and associated connection mixin)
to include information about supported project merge modes.  With
this, if a project on github has the "squash" merge mode disabled
and a Zuul user attempts to configure Zuul to use the "squash"
mode, then Zuul will report a configuration syntax error.

This change adds implementation support only to the github driver.
Other drivers may add support in the future.

For all other drivers, the branch cache mixin simply returns a value
indicating that all merge modes are supported, so there will be no
behavior change.

This is also the upgrade strategy: the branch cache uses a
defaultdict that reports all merge modes supported for any project
when it first loads the cache from ZK after an upgrade.

Change-Id: I3ed9a98dfc1ed63ac11025eb792c61c9a6414384
This commit is contained in:
James E. Blair 2022-11-09 16:22:34 -08:00
parent 1245d100ca
commit 640059a67a
13 changed files with 401 additions and 15 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
doc/source/developer/model-changelog.rst
View File

@ -100,3 +100,9 @@ Version 10
:Prior Zuul version: 6.4.0
:Description: Renames admin_rules to authz_rules in unparsed abide.
Affects schedulers and web.
Version 11
----------
:Prior Zuul version: 8.0.1
:Description: Adds merge_modes to branch cache. Affects schedulers and web.

17
tests/fakegithub.py
View File

@ -238,6 +238,11 @@ class FakeRepository(object):
# List of branch protection rules
self._branch_protection_rules = defaultdict(FakeBranchProtectionRule)
self._repodata = {
'allow_merge_commit': True,
'allow_squash_merge': True,
'allow_rebase_merge': True,
}
# fail the next commit requests with 404
self.fail_not_found = 0
@ -350,6 +355,8 @@ class FakeRepository(object):
return self.get_url_collaborators(request)
if entity == 'commits':
return self.get_url_commits(request, params=params)
if entity == '':
return self.get_url_repo()
else:
return None
@ -444,6 +451,9 @@ class FakeRepository(object):
}
return FakeResponse(data)
def get_url_repo(self):
return FakeResponse(self._repodata)
def pull_requests(self, state=None, sort=None, direction=None):
# sort and direction are unused currently, but present to match
# real world call signatures.
@ -752,7 +762,12 @@ class FakeGithubSession(object):
return FakeResponse(check_run.as_dict(), 200)
def get_repo(self, request, params=None):
org, project, request = request.split('/', 2)
parts = request.split('/', 2)
if len(parts) == 2:
org, project = parts
request = ''
else:
org, project, request = parts
project_name = '{}/{}'.format(org, project)
repo = self.client.repo_from_project(project_name)

21
tests/fixtures/layouts/github-merge-mode.yaml vendored Normal file
View File

@ -0,0 +1,21 @@
- pipeline:
name: check
manager: independent
trigger:
github:
- event: pull_request_review
action: submitted
state: approve
start:
github: {}
success:
github: {}
failure:
github: {}
- project:
name: org/project
merge-mode: rebase
gate:
jobs:
- noop

21
tests/unit/test_github_driver.py
View File

@ -1451,6 +1451,27 @@ class TestGithubDriver(ZuulTestCase):
self.assertEqual('SUCCESS',
self.getJobFromHistory('project-test2').result)
@simple_layout('layouts/github-merge-mode.yaml', driver='github')
def test_merge_method_syntax_check(self):
"""
Tests that the merge mode gets forwarded to the reporter and the
PR was rebased.
"""
github = self.fake_github.getGithubClient()
repo = github.repo_from_project('org/project')
repo._repodata['allow_rebase_merge'] = False
self.scheds.execute(lambda app: app.sched.reconfigure(app.config))
self.waitUntilSettled()
tenant = self.scheds.first.sched.abide.tenants.get('tenant-one')
loading_errors = tenant.layout.loading_errors
self.assertEquals(
len(tenant.layout.loading_errors), 1,
"An error should have been stored")
self.assertIn(
"rebase not supported",
str(loading_errors[0].error))
class TestMultiGithubDriver(ZuulTestCase):
config_file = 'zuul-multi-github.conf'

80
tests/unit/test_model_upgrade.py
View File

@ -17,6 +17,7 @@ import json
from zuul.zk.components import ComponentRegistry
from tests.base import ZuulTestCase, simple_layout, iterate_timeout
from tests.base import ZuulWebFixture
def model_version(version):
@ -305,6 +306,85 @@ class TestGithubModelUpgrade(ZuulTestCase):
], ordered=False)
self.assertTrue(A.is_merged)
@model_version(10)
@simple_layout('layouts/github-merge-mode.yaml', driver='github')
def test_merge_method_syntax_check(self):
"""
Tests that the merge mode gets forwarded to the reporter and the
PR was rebased.
"""
webfixture = self.useFixture(
ZuulWebFixture(self.changes, self.config,
self.additional_event_queues, self.upstream_root,
self.poller_events,
self.git_url_with_auth, self.addCleanup,
self.test_root))
sched = self.scheds.first.sched
web = webfixture.web
github = self.fake_github.getGithubClient()
repo = github.repo_from_project('org/project')
repo._repodata['allow_rebase_merge'] = False
self.scheds.execute(lambda app: app.sched.reconfigure(app.config))
self.waitUntilSettled()
# Verify that there are no errors with model version 9 (we
# should be using the defaultdict that indicates all merge
# modes are supported).
tenant = sched.abide.tenants.get('tenant-one')
self.assertEquals(len(tenant.layout.loading_errors), 0)
# Upgrade our component
self.model_test_component_info.model_api = 11
# Perform a smart reconfiguration which should not clear the
# cache; we should continue to see no errors because we should
# still be using the defaultdict.
self.scheds.first.smartReconfigure()
tenant = sched.abide.tenants.get('tenant-one')
self.assertEquals(len(tenant.layout.loading_errors), 0)
# Wait for web to have the same config
for _ in iterate_timeout(10, "config is synced"):
if (web.tenant_layout_state.get('tenant-one') ==
web.local_layout_state.get('tenant-one')):
break
# Repeat the check
tenant = web.abide.tenants.get('tenant-one')
self.assertEquals(len(tenant.layout.loading_errors), 0)
# Perform a full reconfiguration which should cause us to
# actually query, update the branch cache, and report an
# error.
self.scheds.first.fullReconfigure()
self.waitUntilSettled()
tenant = sched.abide.tenants.get('tenant-one')
loading_errors = tenant.layout.loading_errors
self.assertEquals(
len(tenant.layout.loading_errors), 1,
"An error should have been stored in sched")
self.assertIn(
"rebase not supported",
str(loading_errors[0].error))
# Wait for web to have the same config
for _ in iterate_timeout(10, "config is synced"):
if (web.tenant_layout_state.get('tenant-one') ==
web.local_layout_state.get('tenant-one')):
break
# Repoat the check for web
tenant = web.abide.tenants.get('tenant-one')
loading_errors = tenant.layout.loading_errors
self.assertEquals(
len(tenant.layout.loading_errors), 1,
"An error should have been stored in web")
self.assertIn(
"rebase not supported",
str(loading_errors[0].error))
class TestDeduplication(ZuulTestCase):
config_file = "zuul-gerrit-github.conf"

16
zuul/configloader.py
View File

@ -1858,6 +1858,9 @@ class TenantParser(object):
tpc.branches = static_branches
tpc.dynamic_branches = always_dynamic_branches
tpc.merge_modes = tpc.project.source.getProjectMergeModes(
tpc.project, tenant, min_ltime)
def _loadProjectKeys(self, connection_name, project):
project.private_secrets_key, project.public_secrets_key = (
self.keystorage.getProjectSecretsKeys(
@ -2577,11 +2580,22 @@ class TenantParser(object):
# Set a merge mode if we don't have one for this project.
# This can happen if there are only regex project stanzas
# but no specific project stanzas.
(trusted, project) = tenant.getProject(project_name)
project_metadata = layout.getProjectMetadata(project_name)
if project_metadata.merge_mode is None:
(trusted, project) = tenant.getProject(project_name)
mode = project.source.getProjectDefaultMergeMode(project)
project_metadata.merge_mode = model.MERGER_MAP[mode]
tpc = tenant.project_configs[project.canonical_name]
if tpc.merge_modes is not None:
source_context = model.ProjectContext(
project.canonical_name, project.name)
with project_configuration_exceptions(source_context,
layout.loading_errors):
if project_metadata.merge_mode not in tpc.merge_modes:
mode = model.get_merge_mode_name(
project_metadata.merge_mode)
raise Exception(f'Merge mode {mode} not supported '
f'by project {project_name}')
def _parseLayout(self, tenant, data, loading_errors, layout_uuid=None):
# Don't call this method from dynamic reconfiguration because

103
zuul/connection/__init__.py
View File

@ -15,10 +15,8 @@
import abc
import logging
from typing import List, Optional
from zuul.lib.logutil import get_annotated_logger
from zuul.model import Project
from zuul import model
class ReadOnlyBranchCacheError(RuntimeError):
@ -143,8 +141,8 @@ class ZKBranchCacheMixin:
read_only = False
@abc.abstractmethod
def isBranchProtected(self, project_name: str, branch_name: str,
zuul_event_id) -> Optional[bool]:
def isBranchProtected(self, project_name, branch_name,
zuul_event_id):
"""Return if the branch is protected or None if the branch is unknown.
:param str project_name:
@ -157,9 +155,35 @@ class ZKBranchCacheMixin:
pass
@abc.abstractmethod
def _fetchProjectBranches(self, project: Project,
exclude_unprotected: bool) -> List[str]:
pass
def _fetchProjectBranches(self, project, exclude_unprotected):
"""Perform a remote query to determine the project's branches.
Connection subclasses should implement this method.
:param model.Project project:
The project.
:param bool exclude_unprotected:
Whether the query should exclude unprotected branches from
the response.
:returns: A list of branch names.
"""
def _fetchProjectMergeModes(self, project):
"""Perform a remote query to determine the project's supported merge
modes.
Connection subclasses should implement this method if they are
able to determine which merge modes apply for a project. The
default implemantion returns that all merge modes are valid.
:param model.Project project:
The project.
:returns: A list of merge modes as model IDs.
"""
return model.ALL_MERGE_MODES
def clearConnectionCacheOnBranchEvent(self, event):
"""Update event and clear connection cache if needed.
@ -214,8 +238,7 @@ class ZKBranchCacheMixin:
# Update them if we have them
if protected_branches is not None:
protected_branches = self._fetchProjectBranches(
project, True)
protected_branches = self._fetchProjectBranches(project, True)
self._branch_cache.setProjectBranches(
project.name, True, protected_branches)
@ -223,6 +246,10 @@ class ZKBranchCacheMixin:
all_branches = self._fetchProjectBranches(project, False)
self._branch_cache.setProjectBranches(
project.name, False, all_branches)
merge_modes = self._fetchProjectMergeModes(project)
self._branch_cache.setProjectMergeModes(
project.name, merge_modes)
self.log.info("Got branches for %s" % project.name)
def getProjectBranches(self, project, tenant, min_ltime=-1):
@ -282,6 +309,62 @@ class ZKBranchCacheMixin:
return sorted(branches)
def getProjectMergeModes(self, project, tenant, min_ltime=-1):
"""Get the merge modes for the given project.
:param zuul.model.Project project:
The project for which the merge modes are returned.
:param zuul.model.Tenant tenant:
The related tenant.
:param int min_ltime:
The minimum ltime to determine if we need to refresh the cache.
:returns: The list of merge modes by model id.
"""
merge_modes = None
if self._branch_cache:
try:
merge_modes = self._branch_cache.getProjectMergeModes(
project.name, min_ltime)
except LookupError:
if self.read_only:
# A scheduler hasn't attempted to fetch them yet
raise ReadOnlyBranchCacheError(
"Will not fetch merge modes as read-only is set")
else:
merge_modes = None
if merge_modes is not None:
return merge_modes
elif self.read_only:
# A scheduler has previously attempted a fetch, but got
# the None due to an error; we can't retry since we're
# read-only.
raise RuntimeError(
"Will not fetch merge_modes as read-only is set")
# We need to perform a query
try:
merge_modes = self._fetchProjectMergeModes(project)
except Exception:
# We weren't able to get the merge modes. We need to tell
# future schedulers to try again but tell zuul-web that we
# tried and failed. Set the merge modes to None to indicate
# that we have performed a fetch and retrieved no data. Any
# time we encounter None in the cache, we will try again.
if self._branch_cache:
self._branch_cache.setProjectMergeModes(
project.name, None)
raise
self.log.info("Got merge modes for %s" % project.name)
if self._branch_cache:
self._branch_cache.setProjectMergeModes(
project.name, merge_modes)
return merge_modes
def checkBranchCache(self, project_name: str, event,
protected: bool = None) -> None:
"""Clear the cache for a project when a branch event is processed

46
zuul/driver/github/githubconnection.py
View File

@ -48,6 +48,7 @@ from zuul.driver.github.graphql import GraphQLClient
from zuul.lib import tracing
from zuul.web.handler import BaseWebController
from zuul.lib.logutil import get_annotated_logger
from zuul import model
from zuul.model import Ref, Branch, Tag, Project
from zuul.exceptions import MergeFailure
from zuul.driver.github.githubmodel import PullRequest, GithubTriggerEvent
@ -64,6 +65,12 @@ GITHUB_BASE_URL = 'https://api.github.com'
PREVIEW_JSON_ACCEPT = 'application/vnd.github.machine-man-preview+json'
PREVIEW_DRAFT_ACCEPT = 'application/vnd.github.shadow-cat-preview+json'
PREVIEW_CHECKS_ACCEPT = 'application/vnd.github.antiope-preview+json'
ALL_MERGE_MODES = [
model.MERGER_MERGE,
model.MERGER_MERGE_RESOLVE,
model.MERGER_SQUASH_MERGE,
model.MERGER_REBASE,
]
# NOTE (felix): Using log levels for file comments / annotations is IMHO more
# convenient than the values Github expects. Having in mind that those comments
@ -956,8 +963,9 @@ class GithubClientManager:
if 'cache-control' in response.headers:
del response.headers['cache-control']
self._cache = DictCache()
self.cache_adapter = cachecontrol.CacheControlAdapter(
DictCache(),
self._cache,
cache_etags=True,
heuristic=NoAgeHeuristic())
@ -1774,6 +1782,42 @@ class GithubConnection(ZKChangeCacheMixin, ZKBranchCacheMixin, BaseConnection):
return branches
def _fetchProjectMergeModes(self, project):
github = self.getGithubClient(project.name)
url = github.session.build_url('repos', project.name)
headers = {'Accept': 'application/vnd.github.loki-preview+json'}
merge_modes = []
# GitHub API bug: if the allow_* attributes below are changed,
# the ETag is not updated, meaning that once we cache the repo
# URL, we'll never update it. To avoid this, clear this URL
# from the cache before performing the request.
self._github_client_manager._cache.data.pop(url, None)
resp = github.session.get(url, headers=headers)
if resp.status_code == 403:
self.log.error(str(resp))
rate_limit = github.rate_limit()
if rate_limit['resources']['core']['remaining'] == 0:
self.log.warning(
"Rate limit exceeded, using full merge method list")
return ALL_MERGE_MODES
elif resp.status_code == 404:
raise Exception("Got status code 404 when fetching "
"project %s" % project.name)
resp = resp.json()
if resp.get('allow_merge_commit'):
merge_modes.append(model.MERGER_MERGE)
merge_modes.append(model.MERGER_MERGE_RESOLVE)
if resp.get('allow_squash_merge'):
merge_modes.append(model.MERGER_SQUASH_MERGE)
if resp.get('allow_rebase_merge'):
merge_modes.append(model.MERGER_REBASE)
return merge_modes
def isBranchProtected(self, project_name: str, branch_name: str,
zuul_event_id=None) -> Optional[bool]:
github = self.getGithubClient(

3
zuul/driver/github/githubsource.py
View File

@ -135,6 +135,9 @@ class GithubSource(BaseSource):
def getProjectBranches(self, project, tenant, min_ltime=-1):
return self.connection.getProjectBranches(project, tenant, min_ltime)
def getProjectMergeModes(self, project, tenant, min_ltime=-1):
return self.connection.getProjectMergeModes(project, tenant, min_ltime)
def getProjectBranchCacheLtime(self):
return self.connection._branch_cache.ltime

2
zuul/model.py
View File

@ -65,6 +65,7 @@ MERGER_MAP = {
'squash-merge': MERGER_SQUASH_MERGE,
'rebase': MERGER_REBASE,
}
ALL_MERGE_MODES = list(MERGER_MAP.values())
PRECEDENCE_NORMAL = 0
PRECEDENCE_LOW = 1
@ -6939,6 +6940,7 @@ class TenantProjectConfig(object):
self.extra_config_dirs = ()
# Load config from a different branch if this is a config project
self.load_branch = None
self.merge_modes = None
def isAlwaysDynamicBranch(self, branch):
if self.always_dynamic_branches is None:

2
zuul/model_api.py
View File

@ -14,4 +14,4 @@
# When making ZK schema changes, increment this and add a record to
# docs/developer/model-changelog.rst
MODEL_API = 10
MODEL_API = 11

16
zuul/source/__init__.py
View File

@ -15,6 +15,8 @@
import abc
import time
from zuul import model
class BaseSource(object, metaclass=abc.ABCMeta):
"""Base class for sources.
@ -183,6 +185,20 @@ class BaseSource(object, metaclass=abc.ABCMeta):
"""
def getProjectMergeModes(self, project, tenant, min_ltime=-1):
"""Get supported merge modes for a project
This method is called very frequently, and should generally
return quickly. The connection is expected to cache merge
modes for all projects queried.
The default implementation indicates that all merge modes are
supported.
"""
return model.ALL_MERGE_MODES
@abc.abstractmethod
def getProjectBranchCacheLtime(self):
"""Return the current ltime of the project branch cache."""

83
zuul/zk/branch_cache.py
View File

@ -13,11 +13,15 @@
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.
import collections
import logging
import json
from zuul.zk.zkobject import ZKContext, ShardedZKObject
from zuul.zk.locks import SessionAwareReadLock, SessionAwareWriteLock, locked
from zuul.zk.components import COMPONENT_REGISTRY
from zuul import model
from kazoo.exceptions import NoNodeError
@ -63,15 +67,28 @@ class BranchCacheZKObject(ShardedZKObject):
def __init__(self):
super().__init__()
self._set(protected={},
remainder={})
remainder={},
merge_modes={})
def serialize(self, context):
data = {
"protected": self.protected,
"remainder": self.remainder,
}
# This is mostly here to enable unit tests of upgrades, it's
# safe to move into the dict above at any time.
if (COMPONENT_REGISTRY.model_api >= 11):
data["merge_modes"] = self.merge_modes
return json.dumps(data, sort_keys=True).encode("utf8")
def deserialize(self, raw, context):
data = super().deserialize(raw, context)
# MODEL_API < 11
if "merge_modes" not in data:
data["merge_modes"] = collections.defaultdict(
lambda: model.ALL_MERGE_MODES)
return data
def _save(self, context, data, create=False):
super()._save(context, data, create)
zstat = context.client.exists(self.getPath())
@ -119,10 +136,12 @@ class BranchCache:
if projects is None:
self.cache.protected.clear()
self.cache.remainder.clear()
self.cache.merge_modes.clear()
else:
for p in projects:
self.cache.protected.pop(p, None)
self.cache.remainder.pop(p, None)
self.cache.merge_modes.pop(p, None)
def getProjectBranches(self, project_name, exclude_unprotected,
min_ltime=-1, default=RAISE_EXCEPTION):
@ -255,6 +274,68 @@ class BranchCache:
if branch not in remainder_branches:
remainder_branches.append(branch)
def getProjectMergeModes(self, project_name,
min_ltime=-1, default=RAISE_EXCEPTION):
"""Get the merge modes for the given project.
Checking the branch cache we need to distinguish three different
cases:
1. cache miss (not queried yet)
2. cache hit (including empty list of merge modes)
3. error when fetching merge modes
If the cache doesn't contain any merge modes for the project and no
default value is provided a LookupError is raised.
If there was an error fetching the merge modes, the return value
will be None.
Otherwise the list of merge modes will be returned.
:param str project_name:
The project for which the merge modes are returned.
:param int min_ltime:
The minimum cache ltime to consider the cache valid.
:param any default:
Optional default value to return if no cache entry exits.
:returns: The list of merge modes by model id, or None if there was
an error when fetching the merge modes.
"""
if self.ltime < min_ltime:
with locked(self.rlock):
self.cache.refresh(self.zk_context)
merge_modes = None
try:
merge_modes = self.cache.merge_modes[project_name]
except KeyError:
if default is RAISE_EXCEPTION:
raise LookupError(
f"No merge modes for project {project_name}")
else:
return default
return merge_modes
def setProjectMergeModes(self, project_name, merge_modes):
"""Set the supported merge modes for the given project.
Use None as a sentinel value for the merge modes to indicate
that there was a fetch error.
:param str project_name:
The project for the merge modes.
:param list[int] merge_modes:
The list of merge modes (by model ID) or None.
"""
with locked(self.wlock):
with self.cache.activeContext(self.zk_context):
self.cache.merge_modes[project_name] = merge_modes
@property
def ltime(self):
return self.cache._zstat.last_modified_transaction_id