diff --git a/doc/source/developer/model-changelog.rst b/doc/source/developer/model-changelog.rst index c6d8fedbd7..a14ff98956 100644 --- a/doc/source/developer/model-changelog.rst +++ b/doc/source/developer/model-changelog.rst @@ -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. diff --git a/tests/fakegithub.py b/tests/fakegithub.py index 47cefd08d5..65d0c9a9a4 100644 --- a/tests/fakegithub.py +++ b/tests/fakegithub.py @@ -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) diff --git a/tests/fixtures/layouts/github-merge-mode.yaml b/tests/fixtures/layouts/github-merge-mode.yaml new file mode 100644 index 0000000000..139db886ad --- /dev/null +++ b/tests/fixtures/layouts/github-merge-mode.yaml @@ -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 diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 90d69a7bc9..13f50389be 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -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' diff --git a/tests/unit/test_model_upgrade.py b/tests/unit/test_model_upgrade.py index 8e4cdc20cb..f392e8c3e6 100644 --- a/tests/unit/test_model_upgrade.py +++ b/tests/unit/test_model_upgrade.py @@ -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" diff --git a/zuul/configloader.py b/zuul/configloader.py index 67d4494c49..08b5176185 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -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 diff --git a/zuul/connection/__init__.py b/zuul/connection/__init__.py index 53be27d385..3d4ceec557 100644 --- a/zuul/connection/__init__.py +++ b/zuul/connection/__init__.py @@ -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 diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 1e3af1e787..9b186a0f3b 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -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( diff --git a/zuul/driver/github/githubsource.py b/zuul/driver/github/githubsource.py index bdc373f793..0a94a17303 100644 --- a/zuul/driver/github/githubsource.py +++ b/zuul/driver/github/githubsource.py @@ -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 diff --git a/zuul/model.py b/zuul/model.py index 4625abe224..caf8f9a212 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -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: diff --git a/zuul/model_api.py b/zuul/model_api.py index 9542cf4153..6c93a51773 100644 --- a/zuul/model_api.py +++ b/zuul/model_api.py @@ -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 diff --git a/zuul/source/__init__.py b/zuul/source/__init__.py index faf97b48b5..dd11aa9b63 100644 --- a/zuul/source/__init__.py +++ b/zuul/source/__init__.py @@ -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.""" diff --git a/zuul/zk/branch_cache.py b/zuul/zk/branch_cache.py index 6fa531fade..0a88721586 100644 --- a/zuul/zk/branch_cache.py +++ b/zuul/zk/branch_cache.py @@ -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