From a03a65bf2a2169a9f68835eb95d81fc13cf18ae8 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Fri, 13 Mar 2026 14:44:22 +0000 Subject: [PATCH 1/9] Fix: Derive REST API host from .gitreview The Gerrit REST API hostname was derived as gerrit.{org}.org instead of reading the host field from .gitreview. This caused REST API calls to fail for organizations using a different hostname convention (e.g. git.opendaylight.org). Add _read_gitreview_host() to read the host from the local .gitreview file (available after actions/checkout) or via raw.githubusercontent.com as a fallback. Use it in derive_gerrit_parameters() with priority: 1. Per-org config file entry 2. .gitreview host field 3. Heuristic gerrit.{org}.org fallback Resolves: https://github.com/lfreleng-actions/github2gerrit-action/issues/157 Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- src/github2gerrit/config.py | 93 +++++++++++++++++++++++++++++++++++-- 1 file changed, 90 insertions(+), 3 deletions(-) diff --git a/src/github2gerrit/config.py b/src/github2gerrit/config.py index 4492177..e1a8814 100644 --- a/src/github2gerrit/config.py +++ b/src/github2gerrit/config.py @@ -49,6 +49,8 @@ import logging import os import re +import urllib.parse +import urllib.request from pathlib import Path from typing import Any from typing import cast @@ -454,11 +456,85 @@ def _is_local_cli_context() -> bool: return not _is_github_actions_context() +def _read_gitreview_host(repository: str | None = None) -> str | None: + """Read the Gerrit host from the .gitreview file. + + Checks the local workspace first (available after actions/checkout), + then falls back to fetching via raw.githubusercontent.com when a + GITHUB_REPOSITORY is set. + + Args: + repository: GitHub repository in owner/repo format (optional). + Falls back to GITHUB_REPOSITORY env var. + + Returns: + The host string from .gitreview, or None if unavailable. + """ + _gitreview_host_re = re.compile(r"(?m)^host=(.+)$") + + # 1. Try local .gitreview (available after actions/checkout) + local_path = Path(".gitreview") + if local_path.exists(): + try: + text = local_path.read_text(encoding="utf-8") + match = _gitreview_host_re.search(text) + if match: + host = match.group(1).strip() + if host: + log.debug( + "Read Gerrit host from local .gitreview: %s", host + ) + return host + except Exception as exc: + log.debug("Failed to read local .gitreview: %s", exc) + + # 2. Try fetching from GitHub via raw URL + repo_full = (repository or os.getenv("GITHUB_REPOSITORY") or "").strip() + if not repo_full or "/" not in repo_full: + return None + + for branch in ("master", "main"): + url = ( + f"https://raw.githubusercontent.com/" + f"{repo_full}/refs/heads/{branch}/.gitreview" + ) + parsed = urllib.parse.urlparse(url) + if ( + parsed.scheme != "https" + or parsed.netloc != "raw.githubusercontent.com" + ): + continue + try: + with urllib.request.urlopen(url, timeout=5) as resp: # noqa: S310 + text = resp.read().decode("utf-8") + match = _gitreview_host_re.search(text) + if match: + host = match.group(1).strip() + if host: + log.debug( + "Read Gerrit host from remote .gitreview (%s): %s", + branch, + host, + ) + return host + except Exception as exc: + log.debug( + "Failed to fetch .gitreview from %s branch: %s", branch, exc + ) + + return None + + def derive_gerrit_parameters( organization: str | None, repository: str | None = None ) -> dict[str, str]: """Derive Gerrit parameters using SSH config, git config, and org fallback. + Priority order for server derivation: + 1. Per-org configuration file entry (GERRIT_SERVER) + 2. .gitreview host field (local file or fetched from GitHub) + 3. Heuristic fallback: gerrit.[org].org + Priority order for credential derivation: 1. SSH config user for gerrit.* hosts (checks generic and specific patterns) 2. Git user email from local git configuration @@ -472,7 +548,7 @@ def derive_gerrit_parameters( Dict with derived parameter values: - GERRIT_SSH_USER_G2G: From SSH config or [org].gh2gerrit - GERRIT_SSH_USER_G2G_EMAIL: From git config or fallback email - - GERRIT_SERVER: Resolved from config or gerrit.[org].org + - GERRIT_SERVER: Resolved from config, .gitreview, or gerrit.[org].org - GERRIT_PROJECT: Derived from repository name if provided """ if not organization: @@ -484,8 +560,19 @@ def derive_gerrit_parameters( config = load_org_config(org) configured_server = config.get("GERRIT_SERVER", "").strip() - # Determine the gerrit server to use for SSH config lookup - gerrit_host = configured_server or f"gerrit.{org}.org" + # Read .gitreview host as intermediate fallback before heuristic + gitreview_host = _read_gitreview_host(repository) + + # Priority: config file > .gitreview > heuristic fallback + gerrit_host = configured_server or gitreview_host or f"gerrit.{org}.org" + + if gitreview_host and not configured_server: + log.debug( + "Using Gerrit host from .gitreview: %s (instead of heuristic " + "gerrit.%s.org)", + gitreview_host, + org, + ) # Derive GERRIT_PROJECT from repository if provided gerrit_project = "" From 32d93f339952a715a66c6fe727cb3cb784555678 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Fri, 13 Mar 2026 14:52:12 +0000 Subject: [PATCH 2/9] Fix: Make cleanup REST failures non-fatal The cleanup_closed_github_prs function re-raised GerritRestError as a fatal GitHub2GerritError, causing the entire job to fail even when the primary sync operation succeeded. Log cleanup REST errors as warnings and return gracefully so the job reports success when the core sync completed. Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- src/github2gerrit/gerrit_pr_closer.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/github2gerrit/gerrit_pr_closer.py b/src/github2gerrit/gerrit_pr_closer.py index 305dd0f..760917a 100644 --- a/src/github2gerrit/gerrit_pr_closer.py +++ b/src/github2gerrit/gerrit_pr_closer.py @@ -1493,17 +1493,16 @@ def cleanup_closed_github_prs( continue except GerritRestError as exc: - # Wrap in GitHub2GerritError to ensure SSL/connection errors - # fail the workflow - log.exception("Failed to perform Gerrit cleanup for closed GitHub PRs") - raise GitHub2GerritError( - ExitCode.GERRIT_CONNECTION_ERROR, - message="❌ Gerrit REST API error during cleanup", - details=str(exc), - original_exception=exc, - ) from exc + # Cleanup failures should not mark the job as failed when the + # primary sync operation succeeded. Log a warning instead of + # raising a fatal GitHub2GerritError. + log.warning("Gerrit REST API error during cleanup (non-fatal): %s", exc) + return 0 except Exception: - log.exception("Failed to perform Gerrit cleanup for closed GitHub PRs") + log.warning( + "Gerrit cleanup for closed GitHub PRs failed (non-fatal)", + exc_info=True, + ) return 0 else: log.info( From 378e0e20bece9c71ef6356f8bbb448b681bdaf76 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Fri, 13 Mar 2026 14:55:48 +0000 Subject: [PATCH 3/9] Fix: Mention CREATE_MISSING in UPDATE error message When an UPDATE operation finds no existing Gerrit change, the error message now tells users they can set CREATE_MISSING=true or use the PR comment command to create a new change instead. Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- src/github2gerrit/cli.py | 5 +++-- src/github2gerrit/core.py | 7 ++++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/github2gerrit/cli.py b/src/github2gerrit/cli.py index 54f461a..e3d0aed 100644 --- a/src/github2gerrit/cli.py +++ b/src/github2gerrit/cli.py @@ -1620,8 +1620,9 @@ def _process_single( progress_tracker=progress_tracker, ) safe_console_print( - " To create a new change, trigger the 'opened' " - "workflow action.", + " To create a new change, set CREATE_MISSING=true " + "or add a '@github2gerrit create missing change' " + "comment on the PR.", style="yellow", progress_tracker=progress_tracker, ) diff --git a/src/github2gerrit/core.py b/src/github2gerrit/core.py index 33336ad..a6cf957 100644 --- a/src/github2gerrit/core.py +++ b/src/github2gerrit/core.py @@ -1296,7 +1296,7 @@ def _enforce_existing_change_for_update( topic = f"GH-{gh.repository_owner}-{repo_name}-{gh.pr_number}" msg = ( - f"UPDATE operation requires existing Gerrit change, but " + f"UPDATE operation requires existing Gerrit change, but " # noqa: S608 f"none found. " f"PR #{gh.pr_number} should have an existing change with " f"topic '{topic}'. " @@ -1304,8 +1304,9 @@ def _enforce_existing_change_for_update( f"1. The PR was not previously processed by GitHub2Gerrit\n" f"2. The Gerrit change was abandoned or deleted\n" f"3. The topic was manually changed in Gerrit\n" - f"Consider using 'opened' event type or check Gerrit for " - f"the change." + f"To create a new change anyway, set CREATE_MISSING=true " + f"or add a '@github2gerrit create missing change' comment " + f"on the PR." ) raise OrchestratorError(msg) From 2e3a47050c45762d40fcb0c8fe14c26e409a1680 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Fri, 13 Mar 2026 15:03:33 +0000 Subject: [PATCH 4/9] Test: Add regression tests for issue #157 Cover all three fixes to prevent regressions: - _read_gitreview_host local/remote parsing (9 tests) - derive_gerrit_parameters .gitreview priority chain (4 tests) - cleanup_closed_github_prs non-fatal REST errors (4 tests) - UPDATE error message mentions CREATE_MISSING (2 tests) Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- tests/test_issue_157_regressions.py | 418 ++++++++++++++++++++++++++++ 1 file changed, 418 insertions(+) create mode 100644 tests/test_issue_157_regressions.py diff --git a/tests/test_issue_157_regressions.py b/tests/test_issue_157_regressions.py new file mode 100644 index 0000000..eb37739 --- /dev/null +++ b/tests/test_issue_157_regressions.py @@ -0,0 +1,418 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2025 The Linux Foundation + +""" +Regression tests for GitHub issue #157. + +Covers three fixes: +1. REST API hostname derived from .gitreview instead of gerrit.{org}.org +2. Cleanup REST failures are non-fatal (don't mark the job as failed) +3. UPDATE error message mentions CREATE_MISSING as a remedy + +See: https://github.com/lfreleng-actions/github2gerrit-action/issues/157 +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock +from unittest.mock import patch + +import pytest + +from github2gerrit.config import _read_gitreview_host +from github2gerrit.config import derive_gerrit_parameters +from github2gerrit.gerrit_pr_closer import cleanup_closed_github_prs +from github2gerrit.gerrit_rest import GerritRestError + + +# --------------------------------------------------------------------------- +# 1. _read_gitreview_host — local file +# --------------------------------------------------------------------------- + + +class TestReadGitreviewHostLocal: + """Tests for reading host from a local .gitreview file.""" + + def test_reads_host_from_local_gitreview( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Host is returned when a valid .gitreview exists in cwd.""" + gitreview = tmp_path / ".gitreview" + gitreview.write_text( + "[gerrit]\nhost=git.opendaylight.org\nport=29418\n" + "project=l2switch.git\n", + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + + result = _read_gitreview_host() + assert result == "git.opendaylight.org" + + def test_returns_none_when_no_local_gitreview( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """None when .gitreview does not exist and no repository given.""" + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + + result = _read_gitreview_host() + assert result is None + + def test_returns_none_for_empty_host( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """None when .gitreview exists but host= line is empty.""" + gitreview = tmp_path / ".gitreview" + gitreview.write_text( + "[gerrit]\nhost=\nport=29418\nproject=test.git\n", + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + + result = _read_gitreview_host() + assert result is None + + def test_returns_none_for_missing_host_key( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """None when .gitreview exists but has no host= line at all.""" + gitreview = tmp_path / ".gitreview" + gitreview.write_text( + "[gerrit]\nport=29418\nproject=test.git\n", + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + + result = _read_gitreview_host() + assert result is None + + def test_strips_whitespace_from_host( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Leading/trailing whitespace on the host value is stripped.""" + gitreview = tmp_path / ".gitreview" + gitreview.write_text( + "[gerrit]\nhost= gerrit.example.org \nport=29418\n", + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + + result = _read_gitreview_host() + assert result == "gerrit.example.org" + + +# --------------------------------------------------------------------------- +# 1b. _read_gitreview_host — remote fallback +# --------------------------------------------------------------------------- + + +class TestReadGitreviewHostRemote: + """Tests for fetching .gitreview from raw.githubusercontent.com.""" + + @patch("github2gerrit.config.urllib.request.urlopen") + def test_fetches_from_master_branch( + self, + mock_urlopen: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Falls back to remote when no local .gitreview exists.""" + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + + mock_response = MagicMock() + mock_response.read.return_value = ( + b"[gerrit]\nhost=git.opendaylight.org\n" + b"port=29418\nproject=aaa.git\n" + ) + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + mock_urlopen.return_value = mock_response + + result = _read_gitreview_host("opendaylight/aaa") + assert result == "git.opendaylight.org" + + @patch("github2gerrit.config.urllib.request.urlopen") + def test_returns_none_when_remote_unavailable( + self, + mock_urlopen: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """None when both local and remote .gitreview are unavailable.""" + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + + mock_urlopen.side_effect = OSError("Network unreachable") + + result = _read_gitreview_host("opendaylight/aaa") + assert result is None + + def test_returns_none_when_no_repository( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """None when no local file and no repository is provided.""" + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + + result = _read_gitreview_host(None) + assert result is None + + def test_returns_none_for_bare_repository_name( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """None when repository has no slash (not owner/repo format).""" + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + + result = _read_gitreview_host("noslash") + assert result is None + + +# --------------------------------------------------------------------------- +# 2. derive_gerrit_parameters uses .gitreview host +# --------------------------------------------------------------------------- + + +class TestDeriveGerritParametersGitreview: + """Verify the server derivation priority chain in derive_gerrit_parameters. + + Priority: config file > .gitreview > heuristic gerrit.{org}.org + """ + + @patch("github2gerrit.config._read_gitreview_host") + @patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") + def test_gitreview_host_used_when_no_config( + self, + mock_derive_creds: MagicMock, + mock_read_gitreview: MagicMock, + ) -> None: + """When no config file entry, .gitreview host wins over heuristic.""" + mock_derive_creds.return_value = (None, None) + mock_read_gitreview.return_value = "git.opendaylight.org" + + derived = derive_gerrit_parameters( + "opendaylight", repository="opendaylight/l2switch" + ) + + assert derived["GERRIT_SERVER"] == "git.opendaylight.org" + # SSH credentials should use the .gitreview host for lookup + mock_derive_creds.assert_called_once_with( + "git.opendaylight.org", "opendaylight" + ) + + @patch("github2gerrit.config._read_gitreview_host") + @patch("github2gerrit.config.load_org_config") + @patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") + def test_config_file_beats_gitreview( + self, + mock_derive_creds: MagicMock, + mock_load_org_config: MagicMock, + mock_read_gitreview: MagicMock, + ) -> None: + """Config file GERRIT_SERVER takes precedence over .gitreview.""" + mock_derive_creds.return_value = (None, None) + mock_read_gitreview.return_value = "git.opendaylight.org" + mock_load_org_config.return_value = { + "GERRIT_SERVER": "custom.gerrit.example.org", + } + + derived = derive_gerrit_parameters( + "opendaylight", repository="opendaylight/l2switch" + ) + + assert derived["GERRIT_SERVER"] == "custom.gerrit.example.org" + + @patch("github2gerrit.config._read_gitreview_host") + @patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") + def test_heuristic_fallback_when_no_gitreview( + self, + mock_derive_creds: MagicMock, + mock_read_gitreview: MagicMock, + ) -> None: + """Falls back to gerrit.{org}.org when .gitreview is unavailable.""" + mock_derive_creds.return_value = (None, None) + mock_read_gitreview.return_value = None + + derived = derive_gerrit_parameters("onap") + + assert derived["GERRIT_SERVER"] == "gerrit.onap.org" + + @patch("github2gerrit.config._read_gitreview_host") + @patch("github2gerrit.ssh_config_parser.derive_gerrit_credentials") + def test_project_derived_from_repository( + self, + mock_derive_creds: MagicMock, + mock_read_gitreview: MagicMock, + ) -> None: + """GERRIT_PROJECT is derived from repository owner/repo format.""" + mock_derive_creds.return_value = (None, None) + mock_read_gitreview.return_value = "git.opendaylight.org" + + derived = derive_gerrit_parameters( + "opendaylight", repository="opendaylight/l2switch" + ) + + assert derived["GERRIT_PROJECT"] == "l2switch" + + +# --------------------------------------------------------------------------- +# 3. cleanup_closed_github_prs — REST errors are non-fatal +# --------------------------------------------------------------------------- + + +class TestCleanupNonFatal: + """Verify that cleanup REST failures don't raise fatal errors.""" + + @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + def test_gerrit_rest_error_returns_zero( + self, mock_build_client: MagicMock + ) -> None: + """GerritRestError during cleanup logs a warning and returns 0.""" + mock_client = MagicMock() + mock_client.get.side_effect = GerritRestError( + "Failed to resolve 'gerrit.opendaylight.org'" + ) + mock_build_client.return_value = mock_client + + # Must NOT raise; should return 0 + result = cleanup_closed_github_prs( + gerrit_server="gerrit.opendaylight.org", + gerrit_project="l2switch", + dry_run=True, + ) + assert result == 0 + + @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + def test_connection_error_returns_zero( + self, mock_build_client: MagicMock + ) -> None: + """Generic connection errors during cleanup return 0.""" + mock_build_client.side_effect = ConnectionError( + "Name resolution failed" + ) + + result = cleanup_closed_github_prs( + gerrit_server="gerrit.opendaylight.org", + gerrit_project="l2switch", + dry_run=True, + ) + assert result == 0 + + @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + def test_timeout_error_returns_zero( + self, mock_build_client: MagicMock + ) -> None: + """Timeout errors during cleanup return 0.""" + mock_client = MagicMock() + mock_client.get.side_effect = GerritRestError("timed out") + mock_build_client.return_value = mock_client + + result = cleanup_closed_github_prs( + gerrit_server="unreachable.example.org", + gerrit_project="project", + dry_run=True, + ) + assert result == 0 + + @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + def test_successful_cleanup_returns_count( + self, mock_build_client: MagicMock + ) -> None: + """Normal operation returns 0 when no changes need abandoning.""" + mock_client = MagicMock() + mock_client.get.return_value = [] + mock_build_client.return_value = mock_client + + result = cleanup_closed_github_prs( + gerrit_server="gerrit.example.org", + gerrit_project="test-project", + dry_run=True, + ) + assert result == 0 + + +# --------------------------------------------------------------------------- +# 4. UPDATE error message mentions CREATE_MISSING +# --------------------------------------------------------------------------- + + +class TestUpdateErrorMentionsCreateMissing: + """Verify the UPDATE-no-existing-change error message includes guidance.""" + + def test_error_message_contains_create_missing(self) -> None: + """The OrchestratorError message should mention CREATE_MISSING.""" + from github2gerrit.core import GerritInfo + from github2gerrit.core import Orchestrator + from github2gerrit.core import OrchestratorError + from github2gerrit.models import GitHubContext + + gh = GitHubContext( + event_name="pull_request", + event_action="synchronize", + event_path=None, + repository="opendaylight/aaa", + repository_owner="opendaylight", + server_url="https://github.com", + run_id="123", + sha="abc123", + base_ref="master", + head_ref="feature", + pr_number=3, + ) + + gerrit = GerritInfo( + host="git.opendaylight.org", + port=29418, + project="aaa", + ) + + # Create a minimal orchestrator (workspace doesn't matter here) + orch = Orchestrator(workspace=Path("/tmp/fake")) # noqa: S108 + + # Mock the internal change lookup to return empty (no existing change) + with ( + patch.object(orch, "_find_existing_change_for_pr", return_value=[]), + pytest.raises(OrchestratorError, match="CREATE_MISSING"), + ): + orch._enforce_existing_change_for_update(gh, gerrit) + + def test_error_message_contains_comment_command(self) -> None: + """The error message should mention the PR comment command.""" + from github2gerrit.core import GerritInfo + from github2gerrit.core import Orchestrator + from github2gerrit.core import OrchestratorError + from github2gerrit.models import GitHubContext + + gh = GitHubContext( + event_name="pull_request", + event_action="reopened", + event_path=None, + repository="opendaylight/aaa", + repository_owner="opendaylight", + server_url="https://github.com", + run_id="456", + sha="def456", + base_ref="master", + head_ref="feature", + pr_number=7, + ) + + gerrit = GerritInfo( + host="git.opendaylight.org", + port=29418, + project="aaa", + ) + + orch = Orchestrator(workspace=Path("/tmp/fake")) # noqa: S108 + + with ( + patch.object(orch, "_find_existing_change_for_pr", return_value=[]), + pytest.raises( + OrchestratorError, + match="@github2gerrit create missing change", + ), + ): + orch._enforce_existing_change_for_update(gh, gerrit) From 469939ce68d70c09df4cc14f4c86ef465ad2ecf1 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Fri, 13 Mar 2026 17:01:32 +0000 Subject: [PATCH 5/9] Fix: Address Copilot review feedback for PR #159 - Fix test patch targets: use github2gerrit.gerrit_rest.build_client_for_host instead of github2gerrit.gerrit_pr_closer.build_client_for_host since cleanup_closed_github_prs() does an in-function import that bypasses module-level patching (4 tests in TestCleanupNonFatal) - Skip _read_gitreview_host() call when configured_server is already present from the config file, avoiding unnecessary filesystem/network I/O on the common path in derive_gerrit_parameters() - Make _gitreview_host_re case-insensitive and allow optional whitespace around '=' to handle INI-style 'host = value' and 'Host = value' forms, consistent with extract_gerrit_info_from_gitreview() in ssh_discovery.py - Add regression tests for whitespace and case-insensitive host key parsing - Add exc_info=True to GerritRestError warning log in cleanup for diagnosability, consistent with the generic Exception handler below it - Replace hard-coded /tmp/fake workspace paths with tmp_path fixture in TestUpdateErrorMentionsCreateMissing for cross-platform portability - Fix basedpyright errors across core.py and ssh_discovery.py: - Dedent reconciliation block out of for-trailer loop (change_ids unbound) - Move args and collected_change_ids init before try block (unbound in except handler) - Move ssh_cmd init before try block (unbound in except handler) - Move cmd init before try block in ssh_discovery.py (unbound in except) - Suppress reportUnnecessaryIsInstance on defensive IPv6Address isinstance - Simplify ssh_cmd locals() guard now that variable is always initialised Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- src/github2gerrit/config.py | 26 ++++-- src/github2gerrit/core.py | 52 ++++++------ src/github2gerrit/gerrit_pr_closer.py | 6 +- src/github2gerrit/ssh_discovery.py | 24 +++--- tests/test_issue_157_regressions.py | 110 ++++++++++++++++++++++++-- 5 files changed, 167 insertions(+), 51 deletions(-) diff --git a/src/github2gerrit/config.py b/src/github2gerrit/config.py index e1a8814..b341942 100644 --- a/src/github2gerrit/config.py +++ b/src/github2gerrit/config.py @@ -470,7 +470,7 @@ def _read_gitreview_host(repository: str | None = None) -> str | None: Returns: The host string from .gitreview, or None if unavailable. """ - _gitreview_host_re = re.compile(r"(?m)^host=(.+)$") + _gitreview_host_re = re.compile(r"(?mi)^host[ \t]*=[ \t]*(.+)$") # 1. Try local .gitreview (available after actions/checkout) local_path = Path(".gitreview") @@ -493,10 +493,23 @@ def _read_gitreview_host(repository: str | None = None) -> str | None: if not repo_full or "/" not in repo_full: return None - for branch in ("master", "main"): + # Try PR head/base refs first, then common default branches + branches: list[str] = [] + for env_var in ("GITHUB_HEAD_REF", "GITHUB_BASE_REF"): + ref = (os.getenv(env_var) or "").strip() + if ref: + branches.append(ref) + branches.extend(["master", "main"]) + + tried: set[str] = set() + for branch in branches: + if not branch or branch in tried: + continue + tried.add(branch) + safe_branch = urllib.parse.quote(branch, safe="/") url = ( f"https://raw.githubusercontent.com/" - f"{repo_full}/refs/heads/{branch}/.gitreview" + f"{repo_full}/refs/heads/{safe_branch}/.gitreview" ) parsed = urllib.parse.urlparse(url) if ( @@ -560,8 +573,11 @@ def derive_gerrit_parameters( config = load_org_config(org) configured_server = config.get("GERRIT_SERVER", "").strip() - # Read .gitreview host as intermediate fallback before heuristic - gitreview_host = _read_gitreview_host(repository) + # Read .gitreview host only when no config file entry + # (avoid unnecessary I/O) + gitreview_host = ( + _read_gitreview_host(repository) if not configured_server else None + ) # Priority: config file > .gitreview > heuristic fallback gerrit_host = configured_server or gitreview_host or f"gerrit.{org}.org" diff --git a/src/github2gerrit/core.py b/src/github2gerrit/core.py index a6cf957..52a81a3 100644 --- a/src/github2gerrit/core.py +++ b/src/github2gerrit/core.py @@ -1344,19 +1344,19 @@ def _perform_robust_reconciliation( expected_github_hash = trailer.split(":", 1)[1].strip() break - # Check if this is an update operation - operation_mode = os.getenv("G2G_OPERATION_MODE", "unknown") - is_update_op = operation_mode == "update" + # Check if this is an update operation + operation_mode = os.getenv("G2G_OPERATION_MODE", "unknown") + is_update_op = operation_mode == "update" - change_ids = perform_reconciliation( - inputs=inputs, - gh=gh, - gerrit=gerrit, - local_commits=local_commits, - expected_pr_url=expected_pr_url, - expected_github_hash=expected_github_hash or None, - is_update_operation=is_update_op, - ) + change_ids = perform_reconciliation( + inputs=inputs, + gh=gh, + gerrit=gerrit, + local_commits=local_commits, + expected_pr_url=expected_pr_url, + expected_github_hash=expected_github_hash or None, + is_update_operation=is_update_op, + ) # Store lightweight plan snapshot (only fields needed for verify) try: self._reconciliation_plan = { @@ -3885,17 +3885,18 @@ def _push_to_gerrit( # Use our specific SSH configuration env = self._ssh_env() + args = [ + "git", + "review", + "--yes", + "-v", + "-t", + topic, + ] + log.debug("Building git review command with topic: %s", topic) + collected_change_ids: list[str] = [] + try: - args = [ - "git", - "review", - "--yes", - "-v", - "-t", - topic, - ] - log.debug("Building git review command with topic: %s", topic) - collected_change_ids: list[str] = [] if prepared: collected_change_ids.extend(prepared.all_change_ids()) # Add any Change-Ids captured from apply_pr path (squash amend) @@ -4944,7 +4945,7 @@ def _validate_hostname_against_ssrf(self, hostname: str) -> None: blocked_ips.append(ip_str) # Additional IPv6 specific checks - elif isinstance(ip_obj, ipaddress.IPv6Address) and ( + elif isinstance(ip_obj, ipaddress.IPv6Address) and ( # pyright: ignore[reportUnnecessaryIsInstance] ip_obj in ipaddress.IPv6Network("::1/128") # Loopback or ip_obj in ipaddress.IPv6Network("fe80::/10") # Link-local @@ -5445,9 +5446,10 @@ def _has_existing_backref(commit_sha: str) -> bool: if not csha: log.debug("Empty commit SHA, skipping") continue + # Build SSH command based on available authentication method + ssh_cmd: list[str] = [] try: log.debug("Executing SSH command for commit %s", csha) - # Build SSH command based on available authentication method if self._ssh_key_path and self._ssh_known_hosts_path: # File-based SSH authentication ssh_cmd = [ @@ -5598,7 +5600,7 @@ def _has_existing_backref(commit_sha: str) -> bool: log.debug("SSH stdout: %s", exc.stdout) log.debug( "SSH command that failed: %s", - " ".join(ssh_cmd) if "ssh_cmd" in locals() else "unknown", + " ".join(ssh_cmd) if ssh_cmd else "unknown", ) log.debug( "Back-reference comment failed but change was successfully " diff --git a/src/github2gerrit/gerrit_pr_closer.py b/src/github2gerrit/gerrit_pr_closer.py index 760917a..98af08e 100644 --- a/src/github2gerrit/gerrit_pr_closer.py +++ b/src/github2gerrit/gerrit_pr_closer.py @@ -1496,7 +1496,11 @@ def cleanup_closed_github_prs( # Cleanup failures should not mark the job as failed when the # primary sync operation succeeded. Log a warning instead of # raising a fatal GitHub2GerritError. - log.warning("Gerrit REST API error during cleanup (non-fatal): %s", exc) + log.warning( + "Gerrit REST API error during cleanup (non-fatal): %s", + exc, + exc_info=True, + ) return 0 except Exception: log.warning( diff --git a/src/github2gerrit/ssh_discovery.py b/src/github2gerrit/ssh_discovery.py index a02f516..48e50b2 100644 --- a/src/github2gerrit/ssh_discovery.py +++ b/src/github2gerrit/ssh_discovery.py @@ -128,19 +128,19 @@ def fetch_ssh_host_keys( ) log.debug("✅ Host %s:%d is reachable", hostname, port) - try: - # Use ssh-keyscan to fetch all available key types - cmd = [ - "ssh-keyscan", - "-p", - str(port), - "-T", - str(timeout), - "-t", - "rsa,ecdsa,ed25519", - hostname, - ] + # Use ssh-keyscan to fetch all available key types + cmd = [ + "ssh-keyscan", + "-p", + str(port), + "-T", + str(timeout), + "-t", + "rsa,ecdsa,ed25519", + hostname, + ] + try: log.debug("🔍 Running command: %s", " ".join(cmd)) result = run_cmd(cmd, timeout=timeout + 5) diff --git a/tests/test_issue_157_regressions.py b/tests/test_issue_157_regressions.py index eb37739..00fefea 100644 --- a/tests/test_issue_157_regressions.py +++ b/tests/test_issue_157_regressions.py @@ -102,6 +102,35 @@ def test_strips_whitespace_from_host( result = _read_gitreview_host() assert result == "gerrit.example.org" + def test_reads_host_with_spaces_around_equals( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Host is parsed when .gitreview uses 'host = value' with spaces.""" + gitreview = tmp_path / ".gitreview" + gitreview.write_text( + "[gerrit]\nhost = git.opendaylight.org\nport = 29418\n" + "project = aaa.git\n", + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + + result = _read_gitreview_host() + assert result == "git.opendaylight.org" + + def test_reads_host_case_insensitive( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """Host is parsed when key uses different casing (e.g. 'Host').""" + gitreview = tmp_path / ".gitreview" + gitreview.write_text( + "[gerrit]\nHost = gerrit.example.org\nPort = 29418\n", + encoding="utf-8", + ) + monkeypatch.chdir(tmp_path) + + result = _read_gitreview_host() + assert result == "gerrit.example.org" + # --------------------------------------------------------------------------- # 1b. _read_gitreview_host — remote fallback @@ -170,6 +199,67 @@ def test_returns_none_for_bare_repository_name( result = _read_gitreview_host("noslash") assert result is None + @patch("github2gerrit.config.urllib.request.urlopen") + def test_prefers_github_head_ref_over_master( + self, + mock_urlopen: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """GITHUB_HEAD_REF is tried before master/main branches.""" + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + monkeypatch.setenv("GITHUB_HEAD_REF", "feature/my-branch") + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + mock_response = MagicMock() + mock_response.read.return_value = ( + b"[gerrit]\nhost=gerrit.example.org\nport=29418\nproject=test.git\n" + ) + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + mock_urlopen.return_value = mock_response + + result = _read_gitreview_host("example/repo") + assert result == "gerrit.example.org" + + # Verify the first URL tried uses the head ref, not master + first_call_url = mock_urlopen.call_args_list[0][0][0] + assert "feature/my-branch" in first_call_url + + @patch("github2gerrit.config.urllib.request.urlopen") + def test_tries_base_ref_after_head_ref( + self, + mock_urlopen: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """GITHUB_BASE_REF is tried after GITHUB_HEAD_REF.""" + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + monkeypatch.setenv("GITHUB_HEAD_REF", "feature/branch") + monkeypatch.setenv("GITHUB_BASE_REF", "develop") + + # First call (head ref) fails, second call (base ref) succeeds + mock_response = MagicMock() + mock_response.read.return_value = ( + b"[gerrit]\nhost=gerrit.dev.org\nport=29418\nproject=test.git\n" + ) + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + mock_urlopen.side_effect = [ + OSError("Not found"), + mock_response, + ] + + result = _read_gitreview_host("example/repo") + assert result == "gerrit.dev.org" + + # Verify both refs were tried in order + urls = [call[0][0] for call in mock_urlopen.call_args_list] + assert "feature/branch" in urls[0] + assert "develop" in urls[1] + # --------------------------------------------------------------------------- # 2. derive_gerrit_parameters uses .gitreview host @@ -266,7 +356,7 @@ def test_project_derived_from_repository( class TestCleanupNonFatal: """Verify that cleanup REST failures don't raise fatal errors.""" - @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + @patch("github2gerrit.gerrit_rest.build_client_for_host") def test_gerrit_rest_error_returns_zero( self, mock_build_client: MagicMock ) -> None: @@ -285,7 +375,7 @@ def test_gerrit_rest_error_returns_zero( ) assert result == 0 - @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + @patch("github2gerrit.gerrit_rest.build_client_for_host") def test_connection_error_returns_zero( self, mock_build_client: MagicMock ) -> None: @@ -301,7 +391,7 @@ def test_connection_error_returns_zero( ) assert result == 0 - @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + @patch("github2gerrit.gerrit_rest.build_client_for_host") def test_timeout_error_returns_zero( self, mock_build_client: MagicMock ) -> None: @@ -317,7 +407,7 @@ def test_timeout_error_returns_zero( ) assert result == 0 - @patch("github2gerrit.gerrit_pr_closer.build_client_for_host") + @patch("github2gerrit.gerrit_rest.build_client_for_host") def test_successful_cleanup_returns_count( self, mock_build_client: MagicMock ) -> None: @@ -342,7 +432,9 @@ def test_successful_cleanup_returns_count( class TestUpdateErrorMentionsCreateMissing: """Verify the UPDATE-no-existing-change error message includes guidance.""" - def test_error_message_contains_create_missing(self) -> None: + def test_error_message_contains_create_missing( + self, tmp_path: Path + ) -> None: """The OrchestratorError message should mention CREATE_MISSING.""" from github2gerrit.core import GerritInfo from github2gerrit.core import Orchestrator @@ -370,7 +462,7 @@ def test_error_message_contains_create_missing(self) -> None: ) # Create a minimal orchestrator (workspace doesn't matter here) - orch = Orchestrator(workspace=Path("/tmp/fake")) # noqa: S108 + orch = Orchestrator(workspace=tmp_path) # Mock the internal change lookup to return empty (no existing change) with ( @@ -379,7 +471,9 @@ def test_error_message_contains_create_missing(self) -> None: ): orch._enforce_existing_change_for_update(gh, gerrit) - def test_error_message_contains_comment_command(self) -> None: + def test_error_message_contains_comment_command( + self, tmp_path: Path + ) -> None: """The error message should mention the PR comment command.""" from github2gerrit.core import GerritInfo from github2gerrit.core import Orchestrator @@ -406,7 +500,7 @@ def test_error_message_contains_comment_command(self) -> None: project="aaa", ) - orch = Orchestrator(workspace=Path("/tmp/fake")) # noqa: S108 + orch = Orchestrator(workspace=tmp_path) with ( patch.object(orch, "_find_existing_change_for_pr", return_value=[]), From 3aded70d49d8bf39ac97d0e62c9a88d024c86c19 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Sun, 15 Mar 2026 21:56:46 +0000 Subject: [PATCH 6/9] =?UTF-8?q?Fix:=20Update=20PyJWT=202.10.1=20=E2=86=92?= =?UTF-8?q?=202.12.1=20(GHSA-752w-5fwx-jx9f)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PyJWT 2.10.1 has a High severity vulnerability (GHSA-752w-5fwx-jx9f), fixed in 2.12.0. Update transitive dependency (via PyGithub) to 2.12.1 to resolve Grype SBOM scan failure. Signed-off-by: Matthew Watkins --- uv.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/uv.lock b/uv.lock index be0361e..5ff58f9 100644 --- a/uv.lock +++ b/uv.lock @@ -663,11 +663,11 @@ wheels = [ [[package]] name = "pyjwt" -version = "2.10.1" +version = "2.12.1" source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/e7/46/bd74733ff231675599650d3e47f361794b22ef3e3770998dda30d3b63726/pyjwt-2.10.1.tar.gz", hash = "sha256:3cc5772eb20009233caf06e9d8a0577824723b44e6648ee0a2aedb6cf9381953", size = 87785, upload-time = "2024-11-28T03:43:29.933Z" } +sdist = { url = "https://files.pythonhosted.org/packages/c2/27/a3b6e5bf6ff856d2509292e95c8f57f0df7017cf5394921fc4e4ef40308a/pyjwt-2.12.1.tar.gz", hash = "sha256:c74a7a2adf861c04d002db713dd85f84beb242228e671280bf709d765b03672b", size = 102564, upload-time = "2026-03-13T19:27:37.25Z" } wheels = [ - { url = "https://files.pythonhosted.org/packages/61/ad/689f02752eeec26aed679477e80e632ef1b682313be70793d798c1d5fc8f/PyJWT-2.10.1-py3-none-any.whl", hash = "sha256:dcdd193e30abefd5debf142f9adfcdd2b58004e644f25406ffaebd50bd98dacb", size = 22997, upload-time = "2024-11-28T03:43:27.893Z" }, + { url = "https://files.pythonhosted.org/packages/e5/7a/8dd906bd22e79e47397a61742927f6747fe93242ef86645ee9092e610244/pyjwt-2.12.1-py3-none-any.whl", hash = "sha256:28ca37c070cad8ba8cd9790cd940535d40274d22f80ab87f3ac6a713e6e8454c", size = 29726, upload-time = "2026-03-13T19:27:35.677Z" }, ] [package.optional-dependencies] From 86252b6990ffe01534889b67d1ce581868f38ac1 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Mon, 16 Mar 2026 11:27:11 +0000 Subject: [PATCH 7/9] Refactor: Consolidate .gitreview parsing Extract three independent .gitreview implementations from config.py, core.py, and duplicate_detection.py into a single gitreview.py module with unified parsing, fetching, and data model. Fixes: missing URL encoding on branch names in raw URLs, missing branch deduplication, missing refs/heads/ prefix, and regex inconsistency across the three implementations. Adds base_path derivation from a static known-hosts table at parse time. Includes 97 new tests, fixes basedpyright reportInvalidTypeForm errors in reconciliation.py, and removes unused asyncio_default_fixture _loop_scope pytest config option. Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- pyproject.toml | 2 - src/github2gerrit/cli.py | 5 +- src/github2gerrit/config.py | 72 +- src/github2gerrit/core.py | 224 ++-- src/github2gerrit/duplicate_detection.py | 86 +- src/github2gerrit/error_codes.py | 26 +- src/github2gerrit/gitreview.py | 591 ++++++++++ .../orchestrator/reconciliation.py | 18 +- src/github2gerrit/rich_display.py | 17 +- src/github2gerrit/rich_logging.py | 11 +- tests/conftest.py | 6 +- tests/test_gitreview.py | 1011 +++++++++++++++++ tests/test_issue_157_regressions.py | 8 +- uv.lock | 11 - 14 files changed, 1776 insertions(+), 312 deletions(-) create mode 100644 src/github2gerrit/gitreview.py create mode 100644 tests/test_gitreview.py diff --git a/pyproject.toml b/pyproject.toml index 437e6e5..adcc16f 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -102,7 +102,6 @@ dev = [ # Type checking helpers "pytest-mock>=3.15.1", "types-requests>=2.31.0", - "types-click>=7.1.8", "types-urllib3>=1.26.25.14", ] @@ -230,7 +229,6 @@ directory = "coverage_html_report" minversion = "8.0" addopts = "-ra -q --cov=github2gerrit --cov-report=term-missing --cov-report=html" testpaths = ["tests"] -asyncio_default_fixture_loop_scope = "function" markers = [ "integration: marks tests as integration tests (deselect with '-m \"not integration\"')", ] diff --git a/src/github2gerrit/cli.py b/src/github2gerrit/cli.py index e3d0aed..d12021c 100644 --- a/src/github2gerrit/cli.py +++ b/src/github2gerrit/cli.py @@ -22,6 +22,7 @@ from urllib.parse import urlparse import click +import click.core import typer from . import models @@ -845,8 +846,8 @@ def _env_bool_override( param_name: str, env_var: str, current: bool ) -> bool: """Return *current* if the CLI flag was explicit, else parse env.""" - source = ctx.get_parameter_source(param_name) - if source == click.core.ParameterSource.COMMANDLINE: + source = ctx.get_parameter_source(param_name) # pyright: ignore[reportAttributeAccessIssue] + if source == click.core.ParameterSource.COMMANDLINE: # pyright: ignore[reportAttributeAccessIssue] return current env_val = os.getenv(env_var) if env_val is not None: diff --git a/src/github2gerrit/config.py b/src/github2gerrit/config.py index b341942..6744699 100644 --- a/src/github2gerrit/config.py +++ b/src/github2gerrit/config.py @@ -49,8 +49,6 @@ import logging import os import re -import urllib.parse -import urllib.request from pathlib import Path from typing import Any from typing import cast @@ -459,6 +457,9 @@ def _is_local_cli_context() -> bool: def _read_gitreview_host(repository: str | None = None) -> str | None: """Read the Gerrit host from the .gitreview file. + Delegates to the shared :mod:`github2gerrit.gitreview` module which + consolidates all ``.gitreview`` parsing and fetching logic. + Checks the local workspace first (available after actions/checkout), then falls back to fetching via raw.githubusercontent.com when a GITHUB_REPOSITORY is set. @@ -470,72 +471,9 @@ def _read_gitreview_host(repository: str | None = None) -> str | None: Returns: The host string from .gitreview, or None if unavailable. """ - _gitreview_host_re = re.compile(r"(?mi)^host[ \t]*=[ \t]*(.+)$") - - # 1. Try local .gitreview (available after actions/checkout) - local_path = Path(".gitreview") - if local_path.exists(): - try: - text = local_path.read_text(encoding="utf-8") - match = _gitreview_host_re.search(text) - if match: - host = match.group(1).strip() - if host: - log.debug( - "Read Gerrit host from local .gitreview: %s", host - ) - return host - except Exception as exc: - log.debug("Failed to read local .gitreview: %s", exc) - - # 2. Try fetching from GitHub via raw URL - repo_full = (repository or os.getenv("GITHUB_REPOSITORY") or "").strip() - if not repo_full or "/" not in repo_full: - return None - - # Try PR head/base refs first, then common default branches - branches: list[str] = [] - for env_var in ("GITHUB_HEAD_REF", "GITHUB_BASE_REF"): - ref = (os.getenv(env_var) or "").strip() - if ref: - branches.append(ref) - branches.extend(["master", "main"]) - - tried: set[str] = set() - for branch in branches: - if not branch or branch in tried: - continue - tried.add(branch) - safe_branch = urllib.parse.quote(branch, safe="/") - url = ( - f"https://raw.githubusercontent.com/" - f"{repo_full}/refs/heads/{safe_branch}/.gitreview" - ) - parsed = urllib.parse.urlparse(url) - if ( - parsed.scheme != "https" - or parsed.netloc != "raw.githubusercontent.com" - ): - continue - try: - with urllib.request.urlopen(url, timeout=5) as resp: # noqa: S310 - text = resp.read().decode("utf-8") - match = _gitreview_host_re.search(text) - if match: - host = match.group(1).strip() - if host: - log.debug( - "Read Gerrit host from remote .gitreview (%s): %s", - branch, - host, - ) - return host - except Exception as exc: - log.debug( - "Failed to fetch .gitreview from %s branch: %s", branch, exc - ) + from .gitreview import read_gitreview_host - return None + return read_gitreview_host(repository) def derive_gerrit_parameters( diff --git a/src/github2gerrit/core.py b/src/github2gerrit/core.py index 52a81a3..1af32b9 100644 --- a/src/github2gerrit/core.py +++ b/src/github2gerrit/core.py @@ -54,6 +54,9 @@ from .github_api import get_recent_change_ids_from_comments from .github_api import get_repo_from_env from .github_api import iter_open_pulls +from .gitreview import GerritInfo +from .gitreview import fetch_gitreview +from .gitreview import make_gitreview_info from .gitutils import CommandError from .gitutils import GitError from .gitutils import _parse_trailers @@ -182,11 +185,11 @@ def _is_valid_change_id(value: str) -> bool: ) -@dataclass(frozen=True) -class GerritInfo: - host: str - port: int - project: str +# GerritInfo is imported from .gitreview (aliased from GitReviewInfo). +# It retains the same frozen-dataclass interface (host, port, project) plus +# an additional optional ``base_path`` field (default ``None``). +# The top-level import is sufficient for ``from github2gerrit.core import +# GerritInfo`` to keep working — no explicit re-export needed. @dataclass(frozen=True) @@ -1851,20 +1854,6 @@ def _guard_pull_request_context(self, gh: GitHubContext) -> None: raise OrchestratorError(_MSG_MISSING_PR_CONTEXT) log.debug("PR context OK: #%s", gh.pr_number) - def _parse_gitreview_text(self, text: str) -> GerritInfo | None: - host = _match_first_group(r"(?m)^host=(.+)$", text) - port_s = _match_first_group(r"(?m)^port=(\d+)$", text) - proj = _match_first_group(r"(?m)^project=(.+)$", text) - if host and proj: - project = proj.removesuffix(".git") - port = int(port_s) if port_s else 29418 - return GerritInfo( - host=host.strip(), - port=port, - project=project.strip(), - ) - return None - def _read_gitreview( self, path: Path, @@ -1872,122 +1861,103 @@ def _read_gitreview( ) -> GerritInfo | None: """Read .gitreview and return GerritInfo if present. + Delegates to the shared :mod:`github2gerrit.gitreview` module + which consolidates parsing, local reads, GitHub API fetches, + and raw.githubusercontent.com fallbacks in a single place. + Expected keys: host= port= project=.git """ - if not path.exists(): - log.info(".gitreview not found locally; attempting remote fetch") - # If invoked via direct URL or in environments with a token, - # attempt to read .gitreview from the repository using the API. - try: - client = build_client() - repo_obj: Any = get_repo_from_env(client) - # Prefer a specific ref when available; otherwise default branch - ref = os.getenv("GITHUB_HEAD_REF") or os.getenv("GITHUB_SHA") - content = ( - repo_obj.get_contents(".gitreview", ref=ref) - if ref - else repo_obj.get_contents(".gitreview") - ) - text_remote = ( - getattr(content, "decoded_content", b"") or b"" - ).decode("utf-8") - info_remote = self._parse_gitreview_text(text_remote) - if info_remote: - log.debug("Parsed remote .gitreview: %s", info_remote) - return info_remote - log.info("Remote .gitreview missing required keys; ignoring") - except Exception as exc: - log.debug("Remote .gitreview not available: %s", exc) - # Attempt raw.githubusercontent.com as a fallback + from .gitreview import parse_gitreview + + # --- Strategy 1: local file --- + if path.exists(): + # Read file with explicit error handling so IO failures + # produce a distinct message from malformed content. try: - repo_full = ( - ( - gh.repository - if gh - else os.getenv("GITHUB_REPOSITORY", "") - ) - or "" - ).strip() - branches: list[str] = [] - # Prefer PR head/base refs via GitHub API when running - # from a direct URL when a token is available - try: - # When a target URL was provided via CLI, G2G_TARGET_URL - # contains the actual URL string (truthy check) - if ( - gh - and gh.pr_number - and os.getenv("G2G_TARGET_URL") - and (os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN")) - ): - client = build_client() - repo_obj = get_repo_from_env(client) - pr_obj = get_pull(repo_obj, int(gh.pr_number)) - api_head = str( - getattr( - getattr(pr_obj, "head", object()), "ref", "" - ) - or "" - ) - api_base = str( - getattr( - getattr(pr_obj, "base", object()), "ref", "" - ) - or "" - ) - if api_head: - branches.append(api_head) - if api_base: - branches.append(api_base) - except Exception as exc_api: - log.debug( - "Could not resolve PR refs via API for .gitreview: %s", - exc_api, - ) - if gh and gh.head_ref: - branches.append(gh.head_ref) - if gh and gh.base_ref: - branches.append(gh.base_ref) - branches.extend(["master", "main"]) - tried: set[str] = set() - for br in branches: - if not br or br in tried: - continue - tried.add(br) - url = f"https://raw.githubusercontent.com/{repo_full}/refs/heads/{br}/.gitreview" - parsed = urllib.parse.urlparse(url) - if ( - parsed.scheme != "https" - or parsed.netloc != "raw.githubusercontent.com" - ): - continue - log.info("Fetching .gitreview via raw URL: %s", url) - with urllib.request.urlopen(url, timeout=5) as resp: # noqa: S310 - text_remote = resp.read().decode("utf-8") - info_remote = self._parse_gitreview_text(text_remote) - if info_remote: - log.debug("Parsed remote .gitreview: %s", info_remote) - return info_remote - except Exception as exc2: - log.debug("Raw .gitreview fetch failed: %s", exc2) - log.info("Remote .gitreview not available via API or HTTP") - log.info("Falling back to inputs/env") - return None + text = path.read_text(encoding="utf-8") + except OSError as exc: + msg = f"failed to read .gitreview at {path}: {exc}" + raise OrchestratorError(msg) from exc - try: - text = path.read_text(encoding="utf-8") - except Exception as exc: - msg = f"failed to read .gitreview: {exc}" - raise OrchestratorError(msg) from exc - info_local = self._parse_gitreview_text(text) - if not info_local: + info_local = parse_gitreview(text) + if info_local: + if not info_local.project: + msg = "invalid .gitreview: missing host/project" + raise OrchestratorError(msg) + log.debug("Parsed .gitreview: %s", info_local) + return info_local + # File exists and is readable but is malformed msg = "invalid .gitreview: missing host/project" raise OrchestratorError(msg) - log.debug("Parsed .gitreview: %s", info_local) - return info_local + + log.info(".gitreview not found locally; attempting remote fetch") + + # --- Strategy 2: GitHub API (PyGithub) --- + repo_obj: Any | None = None + try: + client = build_client() + repo_obj = get_repo_from_env(client) + except Exception as exc: + log.debug("Could not build GitHub client for API fetch: %s", exc) + + api_ref = os.getenv("GITHUB_HEAD_REF") or os.getenv("GITHUB_SHA") + + # Collect branch names for the raw URL fallback + branches: list[str] = [] + # Prefer PR head/base refs via GitHub API when running from a + # direct URL and a token is available + try: + if ( + gh + and gh.pr_number + and os.getenv("G2G_TARGET_URL") + and (os.getenv("GITHUB_TOKEN") or os.getenv("GH_TOKEN")) + and repo_obj is not None + ): + pr_obj = get_pull(repo_obj, int(gh.pr_number)) + api_head = str( + getattr(getattr(pr_obj, "head", object()), "ref", "") or "" + ) + api_base = str( + getattr(getattr(pr_obj, "base", object()), "ref", "") or "" + ) + if api_head: + branches.append(api_head) + if api_base: + branches.append(api_base) + except Exception as exc_api: + log.debug( + "Could not resolve PR refs via API for .gitreview: %s", + exc_api, + ) + if gh and gh.head_ref: + branches.append(gh.head_ref) + if gh and gh.base_ref: + branches.append(gh.base_ref) + + repo_full = ( + (gh.repository if gh else os.getenv("GITHUB_REPOSITORY", "")) or "" + ).strip() + + info = fetch_gitreview( + skip_local=True, + repo_obj=repo_obj, + api_ref=api_ref, + repo_full=repo_full, + branches=branches, + ) + if info: + if not info.project: + log.warning("Remote .gitreview missing project field; ignoring") + else: + return info + + log.info("Remote .gitreview not available via API or HTTP") + log.info("Falling back to inputs/env") + return None def _derive_repo_names( self, @@ -2068,7 +2038,7 @@ def _resolve_gerrit_info( else: raise OrchestratorError(_MSG_MISSING_GERRIT_PROJECT) - info = GerritInfo(host=host, port=port, project=project) + info = make_gitreview_info(host=host, port=port, project=project) log.debug("Resolved Gerrit info: %s", info) return info diff --git a/src/github2gerrit/duplicate_detection.py b/src/github2gerrit/duplicate_detection.py index 3588a84..d589b3f 100644 --- a/src/github2gerrit/duplicate_detection.py +++ b/src/github2gerrit/duplicate_detection.py @@ -185,19 +185,20 @@ def __init__( self._cutoff_date = datetime.now(UTC) - timedelta(days=lookback_days) self.duplicates_filter = duplicates_filter - def _match_first_group(self, pattern: str, text: str) -> str: - """Extract first regex group match from text.""" - match = re.search(pattern, text) - return match.group(1) if match else "" - def _resolve_gerrit_info_from_env_or_gitreview( self, gh: GitHubContext ) -> tuple[str, str] | None: """Resolve Gerrit host and project from environment or .gitreview file. + Delegates remote ``.gitreview`` fetching to the shared + :mod:`github2gerrit.gitreview` module which consolidates parsing, + URL-encoding, branch deduplication, and URL validation. + Returns: Tuple of (host, project) if found, None otherwise """ + from .gitreview import fetch_gitreview_raw + # First try environment variables (same as core module) gerrit_host = os.getenv("GERRIT_SERVER", "").strip() gerrit_project = os.getenv("GERRIT_PROJECT", "").strip() @@ -205,65 +206,30 @@ def _resolve_gerrit_info_from_env_or_gitreview( if gerrit_host and gerrit_project: return (gerrit_host, gerrit_project) - # Skip local .gitreview check in composite action context + # Skip local .gitreview check in composite action context. # The duplicate detection runs before workspace setup, so there's no - # reliable local .gitreview file to check. Instead, rely on environment - # variables or remote fetching. + # reliable local .gitreview file to check. Instead, rely on + # environment variables or remote fetching. log.debug("Skipping local .gitreview check (composite action context)") - # Try to fetch .gitreview remotely (simplified version of core logic) - try: - repo_full = gh.repository.strip() if gh.repository else "" - if not repo_full: - return None - - # Try a few common branches - branches = [] - if gh.head_ref: - branches.append(gh.head_ref) - if gh.base_ref: - branches.append(gh.base_ref) - branches.extend(["master", "main"]) - - for branch in branches: - if not branch: - continue - - url = f"https://raw.githubusercontent.com/{repo_full}/{branch}/.gitreview" - - parsed = urllib.parse.urlparse(url) - if ( - parsed.scheme != "https" - or parsed.netloc != "raw.githubusercontent.com" - ): - continue - - try: - log.debug("Fetching .gitreview from: %s", url) - with urllib.request.urlopen(url, timeout=5) as resp: - text_remote = resp.read().decode("utf-8") - - host = self._match_first_group( - r"(?m)^host=(.+)$", text_remote - ) - proj = self._match_first_group( - r"(?m)^project=(.+)$", text_remote - ) - - if host and proj: - project = proj.removesuffix(".git") - return (host.strip(), project.strip()) - if host and not proj: - return (host.strip(), "") - - except Exception as exc: - log.debug( - "Failed to fetch .gitreview from %s: %s", url, exc - ) - continue + repo_full = gh.repository.strip() if gh.repository else "" + if not repo_full: + return None - except Exception as exc: - log.debug("Failed to resolve .gitreview remotely: %s", exc) + # Collect branch hints from the GitHub context + branches: list[str] = [] + if gh.head_ref: + branches.append(gh.head_ref) + if gh.base_ref: + branches.append(gh.base_ref) + + info = fetch_gitreview_raw( + repo_full, + branches=branches, + include_env_refs=False, # caller supplies refs explicitly + ) + if info: + return (info.host, info.project) return None diff --git a/src/github2gerrit/error_codes.py b/src/github2gerrit/error_codes.py index d4779e3..30f146b 100644 --- a/src/github2gerrit/error_codes.py +++ b/src/github2gerrit/error_codes.py @@ -293,6 +293,11 @@ def is_github_api_permission_error(exception: Exception) -> bool: return _matches_github_permission_patterns(exception) +def _get_exc_attr(exc: Exception, name: str) -> object: + """Retrieve an arbitrary attribute from an exception (duck typing).""" + return getattr(exc, name, None) + + def _is_github_exception_with_permission_error(exception: Exception) -> bool: """Check if exception is a GitHub API exception with permission error.""" # Check if it's a PyGithub exception type @@ -303,13 +308,13 @@ def _is_github_exception_with_permission_error(exception: Exception) -> bool: and "github" in exception_type.__module__.lower() ): # Check for status code attribute (PyGithub exceptions have this) - if hasattr(exception, "status") and exception.status in (401, 403, 404): + status = _get_exc_attr(exception, "status") + if status in (401, 403, 404): return True # Check for data attribute with status - if hasattr(exception, "data") and isinstance(exception.data, dict): - status = exception.data.get("status") - if status in (401, 403, 404): - return True + data = _get_exc_attr(exception, "data") + if isinstance(data, dict) and data.get("status") in (401, 403, 404): + return True return False @@ -323,12 +328,11 @@ def _has_permission_related_http_status(exception: Exception) -> bool: return True # Check for requests.HTTPError or similar - if hasattr(exception, "response"): - response = exception.response - if hasattr(response, "status_code"): - status = response.status_code - if isinstance(status, int) and status in (401, 403, 404): - return True + response = _get_exc_attr(exception, "response") + if response is not None: + status = getattr(response, "status_code", None) + if isinstance(status, int) and status in (401, 403, 404): + return True return False diff --git a/src/github2gerrit/gitreview.py b/src/github2gerrit/gitreview.py new file mode 100644 index 0000000..43fe721 --- /dev/null +++ b/src/github2gerrit/gitreview.py @@ -0,0 +1,591 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2025 The Linux Foundation +""" +Shared ``.gitreview`` file parsing and fetching utilities. + +This module provides the **single source of truth** for reading, +parsing, and remotely fetching ``.gitreview`` files across the entire +``github2gerrit`` package. Three previously independent implementations +(in ``config.py``, ``core.py``, and ``duplicate_detection.py``) are +consolidated here. + +Design goals +~~~~~~~~~~~~ + +* **Stdlib-only by default** — no mandatory PyGithub / external imports. + The GitHub API fetch path uses lazy imports so that lightweight callers + (e.g. ``config.py``) do not pull in heavy dependencies. +* **Superset of all callers** — every field and behaviour required by + ``config.py`` (host-only), ``core.py`` (host+port+project), and + ``duplicate_detection.py`` (host+project) is supported, plus the + ``base_path`` field used by the sister ``dependamerge`` project. +* **Consistent regex** — a single set of precompiled patterns that + tolerates optional whitespace around ``=`` and is case-insensitive on + keys (both forms seen in the wild). +* **Bug-free fetching** — URL-encodes branch names, deduplicates the + branch list, and validates raw URLs before opening them. +""" + +from __future__ import annotations + +import logging +import os +import re +import urllib.parse +import urllib.request +from collections.abc import Sequence +from dataclasses import dataclass +from pathlib import Path +from typing import Any +from typing import Final + + +log = logging.getLogger(__name__) + +# ─────────────────────────────────────────────────────────────────────── +# Constants +# ─────────────────────────────────────────────────────────────────────── + +DEFAULT_GERRIT_PORT: int = 29418 +"""Default Gerrit SSH port when the ``port=`` line is absent.""" + +# ─────────────────────────────────────────────────────────────────────── +# Precompiled regex patterns for INI-style .gitreview files +# +# These patterns: +# • are multiline (``(?m)``) +# • are case-insensitive on the key name (``(?i)``) +# • tolerate optional horizontal whitespace around ``=`` +# ─────────────────────────────────────────────────────────────────────── + +_HOST_RE = re.compile(r"(?mi)^host[ \t]*=[ \t]*(.+)$") +_PORT_RE = re.compile(r"(?mi)^port[ \t]*=[ \t]*(\d+)$") +_PROJECT_RE = re.compile(r"(?mi)^project[ \t]*=[ \t]*(.+)$") + +# ─────────────────────────────────────────────────────────────────────── +# Data model +# ─────────────────────────────────────────────────────────────────────── + + +@dataclass(frozen=True) +class GitReviewInfo: + """Parsed contents of a ``.gitreview`` file. + + A typical ``.gitreview`` looks like:: + + [gerrit] + host=gerrit.linuxfoundation.org + port=29418 + project=releng/gerrit_to_platform.git + + Attributes: + host: Gerrit server hostname + (e.g. ``gerrit.linuxfoundation.org``). + port: Gerrit SSH port (default 29418). Not used for REST, + but kept for completeness and parity with ``dependamerge``. + project: Gerrit project path **without** the ``.git`` suffix. + base_path: Optional REST API base path derived from well-known + host conventions (e.g. ``"infra"`` for + ``gerrit.linuxfoundation.org``). ``None`` when the host is + not in the known-hosts table — callers may fall back to + dynamic discovery (see ``gerrit_urls.py``). + """ + + host: str + port: int = DEFAULT_GERRIT_PORT + project: str = "" + base_path: str | None = None + + @property + def is_valid(self) -> bool: + """Minimum validity: *host* must be non-empty.""" + return bool(self.host) + + +# Backward-compatible alias so that ``from github2gerrit.core import +# GerritInfo`` (or any other existing import) continues to work after +# callers are migrated to import from this module instead. +GerritInfo = GitReviewInfo +"""Alias for :class:`GitReviewInfo`. + +Existing code throughout ``core.py``, ``orchestrator/reconciliation.py``, +and 16+ test files references ``GerritInfo``. This alias lets them +switch their import to ``from github2gerrit.gitreview import GerritInfo`` +(or keep importing from ``core`` which will re-export it) without any +other code changes. + +``GerritInfo`` and ``GitReviewInfo`` are the **same** frozen dataclass — +the only difference from the old ``GerritInfo(host, port, project)`` is +the addition of the optional ``base_path`` field (default ``None``). +""" + + +# ─────────────────────────────────────────────────────────────────────── +# Well-known Gerrit base paths +# ─────────────────────────────────────────────────────────────────────── + +_KNOWN_BASE_PATHS: dict[str, str] = { + "gerrit.linuxfoundation.org": "infra", +} + + +def derive_base_path(host: str) -> str | None: + """Return the REST API base path for well-known Gerrit hosts. + + Some Gerrit deployments (notably the Linux Foundation's) serve their + REST API and web UI under a sub-path such as ``/infra``. + + This performs a **static, zero-I/O** lookup against a built-in table + of known hosts. For hosts not in the table it returns ``None`` — + callers that need a definitive answer should fall back to the + dynamic HTTP-probe discovery in ``gerrit_urls.py``. + + Args: + host: Gerrit server hostname (will be lowercased for lookup). + + Returns: + Base path string (e.g. ``"infra"``) or ``None`` if the host is + not in the known-hosts table. + """ + return _KNOWN_BASE_PATHS.get(host.lower().strip()) + + +# Keep a private alias so internal callers don't break if they were +# referencing the underscore-prefixed name during development. +_derive_base_path = derive_base_path + + +# ─────────────────────────────────────────────────────────────────────── +# Pure parser +# ─────────────────────────────────────────────────────────────────────── + + +def parse_gitreview(text: str) -> GitReviewInfo | None: + """Parse the raw text of a ``.gitreview`` file. + + The format is a simple INI-style file with a ``[gerrit]`` section + containing ``host=``, ``port=``, and ``project=`` keys. + + This parser is intentionally lenient: + + * Keys are matched case-insensitively. + * Optional whitespace around ``=`` is tolerated. + * The ``[gerrit]`` section header itself is **not** required — + the parser matches the key lines directly. + + Args: + text: Raw text content of a ``.gitreview`` file. + + Returns: + A :class:`GitReviewInfo` if at least ``host`` is present and + non-empty, otherwise ``None``. + """ + host_match = _HOST_RE.search(text) + if not host_match: + log.debug(".gitreview: no host= line found") + return None + + host = host_match.group(1).strip() + if not host: + log.debug(".gitreview: host= line is empty") + return None + + port_match = _PORT_RE.search(text) + port = int(port_match.group(1)) if port_match else DEFAULT_GERRIT_PORT + + project = "" + project_match = _PROJECT_RE.search(text) + if project_match: + project = project_match.group(1).strip().removesuffix(".git") + + base_path = derive_base_path(host) + + info = GitReviewInfo( + host=host, + port=port, + project=project, + base_path=base_path, + ) + log.debug( + "Parsed .gitreview: host=%s, port=%d, project=%s, base_path=%s", + info.host, + info.port, + info.project, + info.base_path, + ) + return info + + +# ─────────────────────────────────────────────────────────────────────── +# Local file reader +# ─────────────────────────────────────────────────────────────────────── + + +def read_local_gitreview(path: Path | None = None) -> GitReviewInfo | None: + """Read and parse a local ``.gitreview`` file. + + Args: + path: Explicit path to the file. When ``None``, defaults to + ``Path(".gitreview")`` (i.e. the current working directory). + + Returns: + A :class:`GitReviewInfo` on success, or ``None`` if the file + does not exist, is unreadable, or lacks required fields. + """ + target = path or Path(".gitreview") + if not target.exists(): + log.debug("Local .gitreview not found: %s", target) + return None + + try: + text = target.read_text(encoding="utf-8") + except Exception as exc: + log.debug("Failed to read local .gitreview %s: %s", target, exc) + return None + + info = parse_gitreview(text) + if info: + log.debug("Read local .gitreview: %s", info) + else: + log.debug("Local .gitreview at %s missing required fields", target) + return info + + +# ─────────────────────────────────────────────────────────────────────── +# Remote fetch: raw.githubusercontent.com (stdlib only) +# ─────────────────────────────────────────────────────────────────────── + + +def _build_branch_list( + *, + extra_branches: Sequence[str] = (), + include_env_refs: bool = True, + default_branches: Sequence[str] = ("master", "main"), +) -> list[str]: + """Build an ordered, deduplicated list of branches to try. + + Priority order: + + 1. ``extra_branches`` (caller-supplied, e.g. from ``GitHubContext``) + 2. ``GITHUB_HEAD_REF`` / ``GITHUB_BASE_REF`` environment variables + (when *include_env_refs* is ``True``) + 3. ``default_branches`` (``master``, ``main``) + + Empty / ``None`` entries and duplicates are silently dropped. + + Args: + extra_branches: Additional branch names to prepend. + include_env_refs: Whether to consult ``GITHUB_HEAD_REF`` and + ``GITHUB_BASE_REF`` from the environment. + default_branches: Fallback branch names appended at the end. + + Returns: + Ordered list of unique, non-empty branch names. + """ + candidates: list[str] = [] + candidates.extend(extra_branches) + + if include_env_refs: + for var in ("GITHUB_HEAD_REF", "GITHUB_BASE_REF"): + ref = (os.getenv(var) or "").strip() + if ref: + candidates.append(ref) + + candidates.extend(default_branches) + + # Deduplicate while preserving order + seen: set[str] = set() + result: list[str] = [] + for branch in candidates: + if branch and branch not in seen: + seen.add(branch) + result.append(branch) + return result + + +def _validate_raw_url(url: str) -> bool: + """Return ``True`` only if *url* is an HTTPS raw.githubusercontent URL.""" + parsed = urllib.parse.urlparse(url) + return ( + parsed.scheme == "https" + and parsed.netloc == "raw.githubusercontent.com" + ) + + +def fetch_gitreview_raw( + repo_full: str, + *, + branches: Sequence[str] = (), + include_env_refs: bool = True, + default_branches: Sequence[str] = ("master", "main"), + timeout: float = 5.0, +) -> GitReviewInfo | None: + """Fetch ``.gitreview`` from ``raw.githubusercontent.com``. + + Iterates over a list of candidate branches (see + :func:`_build_branch_list`) and returns the first successful parse. + + Branch names are URL-encoded (preserving ``/`` for path separators) + and the final URL is validated before opening. + + Args: + repo_full: GitHub repository in ``owner/repo`` format. + branches: Extra branch names to try first (e.g. from + ``GitHubContext.head_ref``). + include_env_refs: Consult ``GITHUB_HEAD_REF`` / + ``GITHUB_BASE_REF`` environment variables. + default_branches: Fallback branch names (default: + ``("master", "main")``). + timeout: HTTP request timeout in seconds. + + Returns: + A :class:`GitReviewInfo` on success, or ``None``. + """ + repo = (repo_full or "").strip() + if not repo or "/" not in repo: + log.debug("fetch_gitreview_raw: invalid repo_full=%r", repo) + return None + + branch_list = _build_branch_list( + extra_branches=branches, + include_env_refs=include_env_refs, + default_branches=default_branches, + ) + + for branch in branch_list: + safe_branch = urllib.parse.quote(branch, safe="/") + url = ( + f"https://raw.githubusercontent.com/" + f"{repo}/refs/heads/{safe_branch}/.gitreview" + ) + if not _validate_raw_url(url): + log.debug("Skipping invalid raw URL: %s", url) + continue + + try: + log.debug("Fetching .gitreview via raw URL: %s", url) + with urllib.request.urlopen(url, timeout=timeout) as resp: # noqa: S310 + text = resp.read().decode("utf-8") + info = parse_gitreview(text) + if info: + log.debug("Fetched .gitreview from branch %s: %s", branch, info) + return info + except Exception as exc: + log.debug( + "Failed to fetch .gitreview from %s branch: %s", branch, exc + ) + + return None + + +# ─────────────────────────────────────────────────────────────────────── +# Remote fetch: GitHub API (requires PyGithub — lazy import) +# ─────────────────────────────────────────────────────────────────────── + + +def fetch_gitreview_github_api( + repo_obj: Any, + *, + ref: str | None = None, +) -> GitReviewInfo | None: + """Fetch ``.gitreview`` via the PyGithub ``Repository.get_contents`` API. + + This is the highest-fidelity remote fetch path: it honours the + repository's default branch automatically and can target any ref. + + Args: + repo_obj: A PyGithub ``Repository`` object (or any object + exposing ``get_contents(path, ref=...)``). + ref: Optional git ref (branch / tag / SHA). When ``None`` the + API returns the default branch content. + + Returns: + A :class:`GitReviewInfo` on success, or ``None``. + """ + info: GitReviewInfo | None = None + try: + content = ( + repo_obj.get_contents(".gitreview", ref=ref) + if ref + else repo_obj.get_contents(".gitreview") + ) + text = (getattr(content, "decoded_content", b"") or b"").decode("utf-8") + if text: + info = parse_gitreview(text) + else: + log.debug("GitHub API: .gitreview is empty") + except Exception as exc: + log.debug("GitHub API .gitreview fetch failed: %s", exc) + + if info: + log.debug("Parsed .gitreview via GitHub API: %s", info) + elif info is None: + log.debug("GitHub API: .gitreview not available or missing fields") + return info + + +# ─────────────────────────────────────────────────────────────────────── +# Unified fetch: tries all strategies in priority order +# ─────────────────────────────────────────────────────────────────────── + + +def fetch_gitreview( + *, + local_path: Path | None = Path(".gitreview"), + skip_local: bool = False, + repo_obj: Any | None = None, + api_ref: str | None = None, + repo_full: str = "", + branches: Sequence[str] = (), + include_env_refs: bool = True, + default_branches: Sequence[str] = ("master", "main"), + raw_timeout: float = 5.0, +) -> GitReviewInfo | None: + """Fetch and parse ``.gitreview`` using all available strategies. + + Strategies are attempted in priority order: + + 1. **Local file** — read from *local_path* (skipped when + *skip_local* is ``True``). + 2. **GitHub API** — call ``repo_obj.get_contents()`` when a + PyGithub ``Repository`` object is provided. + 3. **Raw URL** — fetch from ``raw.githubusercontent.com`` over a + list of candidate branches. + + The first strategy that yields a valid :class:`GitReviewInfo` wins; + subsequent strategies are not attempted. + + Args: + local_path: Path to a local ``.gitreview`` file. Pass ``None`` + to skip the local read (equivalent to *skip_local=True*). + skip_local: Explicitly skip the local file read (useful for + composite-action contexts where no reliable local file + exists). + repo_obj: Optional PyGithub ``Repository`` object for the + GitHub API strategy. + api_ref: Git ref to pass to ``get_contents()`` (branch / tag / + SHA). Ignored when *repo_obj* is ``None``. + repo_full: GitHub repository in ``owner/repo`` format for the + raw URL strategy. + branches: Extra branch names to try in the raw URL strategy. + include_env_refs: Consult ``GITHUB_HEAD_REF`` / + ``GITHUB_BASE_REF`` for the raw URL strategy. + default_branches: Fallback branch names for the raw URL + strategy. + raw_timeout: HTTP timeout for the raw URL strategy. + + Returns: + A :class:`GitReviewInfo` on success, or ``None`` when all + strategies fail or are skipped. + """ + # 1. Local file + if not skip_local and local_path is not None: + info = read_local_gitreview(local_path) + if info: + return info + + # 2. GitHub API (when a repo object is available) + if repo_obj is not None: + info = fetch_gitreview_github_api(repo_obj, ref=api_ref) + if info: + return info + + # 3. Raw URL fallback + info = fetch_gitreview_raw( + repo_full, + branches=branches, + include_env_refs=include_env_refs, + default_branches=default_branches, + timeout=raw_timeout, + ) + if info: + return info + + log.debug("All .gitreview fetch strategies exhausted") + return None + + +# ─────────────────────────────────────────────────────────────────────── +# Convenience: host-only accessor (replaces config._read_gitreview_host) +# ─────────────────────────────────────────────────────────────────────── + + +def read_gitreview_host( + repository: str | None = None, + *, + local_path: Path | None = None, +) -> str | None: + """Return just the Gerrit **host** from ``.gitreview``. + + This is a thin convenience wrapper around :func:`fetch_gitreview` + that mirrors the previous ``config._read_gitreview_host()`` + signature for a smooth migration path. + + Args: + repository: GitHub repository in ``owner/repo`` format + (optional). Falls back to ``GITHUB_REPOSITORY`` env var. + local_path: Explicit path to a local ``.gitreview``. When + ``None`` (the default), ``Path(".gitreview")`` is used. + + Returns: + The ``host`` string, or ``None`` if unavailable. + """ + repo_full = (repository or os.getenv("GITHUB_REPOSITORY") or "").strip() + + info = fetch_gitreview( + local_path=local_path or Path(".gitreview"), + repo_full=repo_full, + ) + return info.host if info else None + + +# ─────────────────────────────────────────────────────────────────────── +# Factory: build GitReviewInfo from explicit parameters +# ─────────────────────────────────────────────────────────────────────── + +_BASE_PATH_UNSET: Final[object] = object() +"""Sentinel value for :func:`make_gitreview_info` to distinguish +"caller did not pass *base_path*" from an explicit ``None``.""" + + +def make_gitreview_info( + host: str, + port: int = DEFAULT_GERRIT_PORT, + project: str = "", + *, + base_path: str | None | object = _BASE_PATH_UNSET, +) -> GitReviewInfo: + """Construct a :class:`GitReviewInfo` from explicit parameters. + + When *base_path* is not supplied the static known-hosts table is + consulted automatically (via :func:`derive_base_path`). Pass + ``base_path=None`` explicitly to suppress this lookup. + + This factory is the preferred way to build ``GitReviewInfo`` (a.k.a. + ``GerritInfo``) instances outside of ``.gitreview`` parsing — for + example when resolving Gerrit connection info from CLI inputs or + environment variables. + + Args: + host: Gerrit server hostname. + port: Gerrit SSH port (default 29418). + project: Gerrit project path (without ``.git`` suffix). + base_path: REST API base path override. When omitted, derived + automatically from *host*. + + Returns: + A frozen :class:`GitReviewInfo` instance. + """ + resolved_base_path: str | None = ( + derive_base_path(host) + if base_path is _BASE_PATH_UNSET + else base_path + if isinstance(base_path, str) + else None + ) + return GitReviewInfo( + host=host, + port=port, + project=project, + base_path=resolved_base_path, + ) diff --git a/src/github2gerrit/orchestrator/reconciliation.py b/src/github2gerrit/orchestrator/reconciliation.py index 72fbbdd..1fba144 100644 --- a/src/github2gerrit/orchestrator/reconciliation.py +++ b/src/github2gerrit/orchestrator/reconciliation.py @@ -36,29 +36,23 @@ import time from collections.abc import Sequence from dataclasses import dataclass +from typing import TYPE_CHECKING -from github2gerrit.core import GerritInfo from github2gerrit.gerrit_query import GerritChange from github2gerrit.gerrit_query import query_changes_by_topic +from github2gerrit.gitreview import GerritInfo from github2gerrit.mapping_comment import parse_mapping_comments from github2gerrit.mapping_comment import validate_mapping_consistency from github2gerrit.reconcile_matcher import LocalCommit from github2gerrit.reconcile_matcher import ReconciliationMatcher -log = logging.getLogger(__name__) - - -# --------------------------------------------------------------------------- -# Type hints (imported lazily to avoid circulars) -# --------------------------------------------------------------------------- - -try: # pragma: no cover - typing only +if TYPE_CHECKING: from github2gerrit.models import GitHubContext from github2gerrit.models import Inputs -except Exception: # pragma: no cover - GitHubContext = object # type: ignore[misc,assignment] - Inputs = object # type: ignore[misc,assignment] + + +log = logging.getLogger(__name__) # --------------------------------------------------------------------------- diff --git a/src/github2gerrit/rich_display.py b/src/github2gerrit/rich_display.py index 8208607..ca55049 100644 --- a/src/github2gerrit/rich_display.py +++ b/src/github2gerrit/rich_display.py @@ -23,17 +23,16 @@ try: - from rich.console import Console - from rich.live import Live - from rich.table import Table - from rich.text import Text + from rich.console import Console # pyright: ignore[reportAssignmentType] + from rich.live import Live # pyright: ignore[reportAssignmentType] + from rich.table import Table # pyright: ignore[reportAssignmentType] + from rich.text import Text # pyright: ignore[reportAssignmentType] RICH_AVAILABLE = True except ImportError: RICH_AVAILABLE = False - # Fallback classes for when Rich is not available - class Live: # type: ignore + class Live: # type: ignore[no-redef] def __init__(self, *args: Any, **kwargs: Any) -> None: pass @@ -46,21 +45,21 @@ def stop(self) -> None: def update(self, *args: Any) -> None: pass - class Text: # type: ignore + class Text: # type: ignore[no-redef] def __init__(self, *args: Any, **kwargs: Any) -> None: pass def append(self, *args: Any, **kwargs: Any) -> None: pass - class Console: # type: ignore + class Console: # type: ignore[no-redef] def __init__(self, *args: Any, **kwargs: Any) -> None: pass def print(self, *args: Any, **kwargs: Any) -> None: print(*args) - class Table: # type: ignore + class Table: # type: ignore[no-redef] def __init__(self, *args: Any, **kwargs: Any) -> None: pass diff --git a/src/github2gerrit/rich_logging.py b/src/github2gerrit/rich_logging.py index 5bfab42..48068d3 100644 --- a/src/github2gerrit/rich_logging.py +++ b/src/github2gerrit/rich_logging.py @@ -26,22 +26,23 @@ try: - from rich.console import Console - from rich.logging import RichHandler + from rich.console import Console # pyright: ignore[reportAssignmentType] + from rich.logging import ( + RichHandler, # pyright: ignore[reportAssignmentType] + ) RICH_AVAILABLE = True except ImportError: RICH_AVAILABLE = False - # Fallback classes for when Rich is not available - class Console: # type: ignore + class Console: # type: ignore[no-redef] def __init__(self, *args: Any, **kwargs: Any) -> None: pass def print(self, *args: Any, **kwargs: Any) -> None: print(*args) - class RichHandler: # type: ignore + class RichHandler: # type: ignore[no-redef] pass diff --git a/tests/conftest.py b/tests/conftest.py index e6b3ff1..a908c6e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -173,7 +173,9 @@ def pytest_sessionstart(session: Any) -> None: @pytest.fixture(autouse=True) -def disable_github_ci_mode(monkeypatch, request): +def disable_github_ci_mode( + monkeypatch: pytest.MonkeyPatch, request: pytest.FixtureRequest +) -> None: """ Automatically disable GitHub CI mode detection for all tests. @@ -201,7 +203,7 @@ def disable_github_ci_mode(monkeypatch, request): @pytest.fixture(autouse=True) -def isolate_git_environment(monkeypatch): +def isolate_git_environment(monkeypatch: pytest.MonkeyPatch) -> None: """ Isolate git environment for each test to prevent cross-test contamination. diff --git a/tests/test_gitreview.py b/tests/test_gitreview.py new file mode 100644 index 0000000..5a9c3b5 --- /dev/null +++ b/tests/test_gitreview.py @@ -0,0 +1,1011 @@ +# SPDX-License-Identifier: Apache-2.0 +# SPDX-FileCopyrightText: 2025 The Linux Foundation +"""Tests for the shared gitreview module. + +Covers: +- GitReviewInfo / GerritInfo data model +- parse_gitreview() — pure text parser +- read_local_gitreview() — local file reader +- _build_branch_list() — branch list construction +- _validate_raw_url() — URL validation +- fetch_gitreview_raw() — raw.githubusercontent.com fetcher +- fetch_gitreview_github_api() — PyGithub API fetcher +- fetch_gitreview() — unified multi-strategy fetcher +- read_gitreview_host() — convenience host-only accessor +- make_gitreview_info() — factory with auto-derived base_path +- derive_base_path() — static known-host lookup +- GerritInfo backward-compatible alias +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock +from unittest.mock import patch + +import pytest + +from github2gerrit.gitreview import DEFAULT_GERRIT_PORT +from github2gerrit.gitreview import GerritInfo +from github2gerrit.gitreview import GitReviewInfo +from github2gerrit.gitreview import _build_branch_list +from github2gerrit.gitreview import _validate_raw_url +from github2gerrit.gitreview import derive_base_path +from github2gerrit.gitreview import fetch_gitreview +from github2gerrit.gitreview import fetch_gitreview_github_api +from github2gerrit.gitreview import fetch_gitreview_raw +from github2gerrit.gitreview import make_gitreview_info +from github2gerrit.gitreview import parse_gitreview +from github2gerrit.gitreview import read_gitreview_host +from github2gerrit.gitreview import read_local_gitreview + + +# ----------------------------------------------------------------------- +# Fixtures +# ----------------------------------------------------------------------- + +TYPICAL_GITREVIEW = ( + "[gerrit]\n" + "host=gerrit.linuxfoundation.org\n" + "port=29418\n" + "project=releng/lftools.git\n" +) + +MINIMAL_GITREVIEW = "[gerrit]\nhost=gerrit.example.org\n" + +SPACES_AROUND_EQUALS = ( + "[gerrit]\nhost = git.opendaylight.org\nport = 29418\nproject = aaa.git\n" +) + +MIXED_CASE_KEYS = ( + "[gerrit]\nHost=gerrit.example.org\nPort=29419\nProject=apps/widgets.git\n" +) + +NO_HOST = "[gerrit]\nport=29418\nproject=foo.git\n" + +EMPTY_HOST = "[gerrit]\nhost=\nport=29418\nproject=foo.git\n" + +NO_PORT = "[gerrit]\nhost=gerrit.example.org\nproject=foo.git\n" + +NO_PROJECT = "[gerrit]\nhost=gerrit.example.org\nport=29418\n" + +WHITESPACE_HOST = "[gerrit]\nhost= gerrit.example.org \n" + +NON_DEFAULT_PORT = ( + "[gerrit]\nhost=gerrit.acme.org\nport=29419\nproject=acme/widgets.git\n" +) + + +# ----------------------------------------------------------------------- +# GitReviewInfo data model +# ----------------------------------------------------------------------- + + +class TestGitReviewInfoModel: + """Tests for the GitReviewInfo frozen dataclass.""" + + def test_default_values(self) -> None: + info = GitReviewInfo(host="h") + assert info.host == "h" + assert info.port == DEFAULT_GERRIT_PORT + assert info.project == "" + assert info.base_path is None + + def test_is_valid_with_host(self) -> None: + assert GitReviewInfo(host="h").is_valid is True + + def test_is_valid_empty_host(self) -> None: + assert GitReviewInfo(host="").is_valid is False + + def test_frozen(self) -> None: + info = GitReviewInfo(host="h") + with pytest.raises(AttributeError): + info.host = "other" # type: ignore[misc] + + def test_equality(self) -> None: + a = GitReviewInfo(host="h", port=1, project="p", base_path="bp") + b = GitReviewInfo(host="h", port=1, project="p", base_path="bp") + assert a == b + + def test_inequality_on_base_path(self) -> None: + a = GitReviewInfo(host="h", base_path=None) + b = GitReviewInfo(host="h", base_path="infra") + assert a != b + + +class TestGerritInfoAlias: + """GerritInfo must be the same class as GitReviewInfo.""" + + def test_alias_identity(self) -> None: + assert GerritInfo is GitReviewInfo + + def test_construct_via_alias(self) -> None: + info = GerritInfo(host="h", port=29418, project="p") + assert isinstance(info, GitReviewInfo) + assert info.host == "h" + + def test_import_from_core_still_works(self) -> None: + """Verify the re-export from core.py is the same class.""" + from github2gerrit.core import GerritInfo as CoreGerritInfo + + assert CoreGerritInfo is GitReviewInfo + + +# ----------------------------------------------------------------------- +# derive_base_path — static known-host lookup +# ----------------------------------------------------------------------- + + +class TestDeriveBasePath: + def test_known_host(self) -> None: + assert derive_base_path("gerrit.linuxfoundation.org") == "infra" + + def test_known_host_case_insensitive(self) -> None: + assert derive_base_path("Gerrit.LinuxFoundation.Org") == "infra" + + def test_known_host_with_whitespace(self) -> None: + assert derive_base_path(" gerrit.linuxfoundation.org ") == "infra" + + def test_unknown_host(self) -> None: + assert derive_base_path("gerrit.example.org") is None + + def test_empty_host(self) -> None: + assert derive_base_path("") is None + + +# ----------------------------------------------------------------------- +# parse_gitreview — pure parser +# ----------------------------------------------------------------------- + + +class TestParseGitreview: + def test_typical(self) -> None: + info = parse_gitreview(TYPICAL_GITREVIEW) + assert info is not None + assert info.host == "gerrit.linuxfoundation.org" + assert info.port == 29418 + assert info.project == "releng/lftools" + assert info.base_path == "infra" + + def test_minimal_host_only(self) -> None: + info = parse_gitreview(MINIMAL_GITREVIEW) + assert info is not None + assert info.host == "gerrit.example.org" + assert info.port == DEFAULT_GERRIT_PORT + assert info.project == "" + assert info.base_path is None + + def test_spaces_around_equals(self) -> None: + info = parse_gitreview(SPACES_AROUND_EQUALS) + assert info is not None + assert info.host == "git.opendaylight.org" + assert info.port == 29418 + assert info.project == "aaa" + + def test_mixed_case_keys(self) -> None: + info = parse_gitreview(MIXED_CASE_KEYS) + assert info is not None + assert info.host == "gerrit.example.org" + assert info.port == 29419 + assert info.project == "apps/widgets" + + def test_no_host_returns_none(self) -> None: + assert parse_gitreview(NO_HOST) is None + + def test_empty_host_returns_none(self) -> None: + assert parse_gitreview(EMPTY_HOST) is None + + def test_no_port_defaults(self) -> None: + info = parse_gitreview(NO_PORT) + assert info is not None + assert info.port == DEFAULT_GERRIT_PORT + + def test_no_project_ok(self) -> None: + info = parse_gitreview(NO_PROJECT) + assert info is not None + assert info.project == "" + + def test_strips_whitespace_from_host(self) -> None: + info = parse_gitreview(WHITESPACE_HOST) + assert info is not None + assert info.host == "gerrit.example.org" + + def test_removes_dot_git_suffix(self) -> None: + info = parse_gitreview(NON_DEFAULT_PORT) + assert info is not None + assert info.project == "acme/widgets" + + def test_non_default_port(self) -> None: + info = parse_gitreview(NON_DEFAULT_PORT) + assert info is not None + assert info.port == 29419 + + def test_empty_string(self) -> None: + assert parse_gitreview("") is None + + def test_garbage_text(self) -> None: + assert parse_gitreview("nothing useful here\n") is None + + def test_base_path_derived_for_lf_host(self) -> None: + text = "[gerrit]\nhost=gerrit.linuxfoundation.org\nproject=foo.git\n" + info = parse_gitreview(text) + assert info is not None + assert info.base_path == "infra" + + def test_base_path_none_for_unknown_host(self) -> None: + text = "[gerrit]\nhost=gerrit.acme.org\nproject=foo.git\n" + info = parse_gitreview(text) + assert info is not None + assert info.base_path is None + + def test_project_without_git_suffix(self) -> None: + text = "[gerrit]\nhost=h\nproject=releng/builder\n" + info = parse_gitreview(text) + assert info is not None + assert info.project == "releng/builder" + + def test_tabs_around_equals(self) -> None: + text = "[gerrit]\nhost\t=\tgerrit.example.org\n" + info = parse_gitreview(text) + assert info is not None + assert info.host == "gerrit.example.org" + + +# ----------------------------------------------------------------------- +# read_local_gitreview — local file reader +# ----------------------------------------------------------------------- + + +class TestReadLocalGitreview: + def test_reads_valid_file(self, tmp_path: Path) -> None: + p = tmp_path / ".gitreview" + p.write_text(TYPICAL_GITREVIEW, encoding="utf-8") + info = read_local_gitreview(p) + assert info is not None + assert info.host == "gerrit.linuxfoundation.org" + + def test_returns_none_when_missing(self, tmp_path: Path) -> None: + assert read_local_gitreview(tmp_path / "nonexistent") is None + + def test_returns_none_for_malformed(self, tmp_path: Path) -> None: + p = tmp_path / ".gitreview" + p.write_text("garbage\n", encoding="utf-8") + assert read_local_gitreview(p) is None + + def test_returns_none_for_unreadable(self, tmp_path: Path) -> None: + p = tmp_path / ".gitreview" + p.write_text(TYPICAL_GITREVIEW, encoding="utf-8") + p.chmod(0o000) + result = read_local_gitreview(p) + # Restore permissions for cleanup + p.chmod(0o644) + assert result is None + + def test_defaults_to_cwd( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + (tmp_path / ".gitreview").write_text( + MINIMAL_GITREVIEW, encoding="utf-8" + ) + monkeypatch.chdir(tmp_path) + info = read_local_gitreview() + assert info is not None + assert info.host == "gerrit.example.org" + + def test_defaults_to_cwd_missing( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.chdir(tmp_path) + assert read_local_gitreview() is None + + +# ----------------------------------------------------------------------- +# _build_branch_list — branch list construction +# ----------------------------------------------------------------------- + + +class TestBuildBranchList: + def test_defaults_only(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + result = _build_branch_list() + assert result == ["master", "main"] + + def test_extra_branches_prepended( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + result = _build_branch_list(extra_branches=["feature/a", "develop"]) + assert result == ["feature/a", "develop", "master", "main"] + + def test_env_refs_included(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "pr-branch") + monkeypatch.setenv("GITHUB_BASE_REF", "main") + result = _build_branch_list() + # "main" should appear once even though it's in env + defaults + assert result == ["pr-branch", "main", "master"] + + def test_env_refs_skipped_when_disabled( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "pr-branch") + monkeypatch.setenv("GITHUB_BASE_REF", "develop") + result = _build_branch_list(include_env_refs=False) + assert result == ["master", "main"] + + def test_deduplication(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "master") + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + result = _build_branch_list(extra_branches=["master"]) + # "master" should appear exactly once + assert result.count("master") == 1 + + def test_empty_entries_filtered( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "") + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + result = _build_branch_list(extra_branches=["", "valid", ""]) + assert "" not in result + assert "valid" in result + + def test_custom_defaults(self, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + result = _build_branch_list(default_branches=["develop", "trunk"]) + assert result == ["develop", "trunk"] + + def test_order_is_extra_then_env_then_defaults( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.setenv("GITHUB_HEAD_REF", "env-head") + monkeypatch.setenv("GITHUB_BASE_REF", "env-base") + result = _build_branch_list( + extra_branches=["first"], + default_branches=["last"], + ) + assert result == ["first", "env-head", "env-base", "last"] + + +# ----------------------------------------------------------------------- +# _validate_raw_url — URL validation +# ----------------------------------------------------------------------- + + +class TestValidateRawUrl: + def test_valid_url(self) -> None: + assert _validate_raw_url( + "https://raw.githubusercontent.com/owner/repo/refs/heads/main/.gitreview" + ) + + def test_http_rejected(self) -> None: + assert not _validate_raw_url( + "http://raw.githubusercontent.com/owner/repo/refs/heads/main/.gitreview" + ) + + def test_wrong_host_rejected(self) -> None: + assert not _validate_raw_url( + "https://evil.example.com/owner/repo/refs/heads/main/.gitreview" + ) + + def test_empty_rejected(self) -> None: + assert not _validate_raw_url("") + + +# ----------------------------------------------------------------------- +# fetch_gitreview_raw — raw.githubusercontent.com fetcher +# ----------------------------------------------------------------------- + + +def _mock_urlopen_for_text(text: str) -> MagicMock: + """Create a mock for urllib.request.urlopen that returns the given text.""" + mock_response = MagicMock() + mock_response.read.return_value = text.encode("utf-8") + mock_response.__enter__ = lambda s: s + mock_response.__exit__ = MagicMock(return_value=False) + return mock_response + + +class TestFetchGitreviewRaw: + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_fetches_first_valid_branch( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + mock_urlopen.return_value = _mock_urlopen_for_text(TYPICAL_GITREVIEW) + + info = fetch_gitreview_raw( + "lfit/releng-lftools", include_env_refs=False + ) + assert info is not None + assert info.host == "gerrit.linuxfoundation.org" + assert info.project == "releng/lftools" + assert info.base_path == "infra" + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_tries_extra_branches_first( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + # First call (for "custom-branch") succeeds + mock_urlopen.return_value = _mock_urlopen_for_text(MINIMAL_GITREVIEW) + + info = fetch_gitreview_raw( + "owner/repo", + branches=["custom-branch"], + include_env_refs=False, + ) + assert info is not None + # Verify the URL used the custom branch + call_args = mock_urlopen.call_args + assert "custom-branch" in call_args[0][0] + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_falls_through_on_failure( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + # First branch fails, second succeeds + mock_urlopen.side_effect = [ + OSError("404 Not Found"), + _mock_urlopen_for_text(MINIMAL_GITREVIEW), + ] + + info = fetch_gitreview_raw("owner/repo", include_env_refs=False) + assert info is not None + assert info.host == "gerrit.example.org" + assert mock_urlopen.call_count == 2 + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_returns_none_when_all_fail( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + mock_urlopen.side_effect = OSError("Network unreachable") + assert fetch_gitreview_raw("owner/repo", include_env_refs=False) is None + + def test_returns_none_for_empty_repo(self) -> None: + assert fetch_gitreview_raw("") is None + + def test_returns_none_for_repo_without_slash(self) -> None: + assert fetch_gitreview_raw("noslash") is None + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_url_encodes_branch_names( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + mock_urlopen.return_value = _mock_urlopen_for_text(MINIMAL_GITREVIEW) + + fetch_gitreview_raw( + "owner/repo", + branches=["feature/my branch"], + include_env_refs=False, + ) + url_used = mock_urlopen.call_args[0][0] + # Space should be encoded as %20, slash preserved + assert "feature/my%20branch" in url_used + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_preserves_slash_in_branch_names( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + mock_urlopen.return_value = _mock_urlopen_for_text(MINIMAL_GITREVIEW) + + fetch_gitreview_raw( + "owner/repo", + branches=["feature/foo"], + include_env_refs=False, + ) + url_used = mock_urlopen.call_args[0][0] + assert "feature/foo" in url_used + assert "feature%2Ffoo" not in url_used + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_deduplicates_branches( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + # All branches fail so we can count attempts + mock_urlopen.side_effect = OSError("fail") + + fetch_gitreview_raw( + "owner/repo", + branches=["master", "main", "master"], + include_env_refs=False, + ) + # Should only try master and main (2), not master twice (3) + assert mock_urlopen.call_count == 2 + + +# ----------------------------------------------------------------------- +# fetch_gitreview_github_api — PyGithub API fetcher +# ----------------------------------------------------------------------- + + +class TestFetchGitreviewGithubApi: + def test_successful_fetch(self) -> None: + mock_content = MagicMock() + mock_content.decoded_content = TYPICAL_GITREVIEW.encode("utf-8") + + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + info = fetch_gitreview_github_api(mock_repo) + assert info is not None + assert info.host == "gerrit.linuxfoundation.org" + mock_repo.get_contents.assert_called_once_with(".gitreview") + + def test_with_ref(self) -> None: + mock_content = MagicMock() + mock_content.decoded_content = MINIMAL_GITREVIEW.encode("utf-8") + + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + info = fetch_gitreview_github_api(mock_repo, ref="develop") + assert info is not None + mock_repo.get_contents.assert_called_once_with( + ".gitreview", ref="develop" + ) + + def test_empty_content(self) -> None: + mock_content = MagicMock() + mock_content.decoded_content = b"" + + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + assert fetch_gitreview_github_api(mock_repo) is None + + def test_404_returns_none(self) -> None: + mock_repo = MagicMock() + mock_repo.get_contents.side_effect = Exception("404 Not Found") + + assert fetch_gitreview_github_api(mock_repo) is None + + def test_malformed_returns_none(self) -> None: + mock_content = MagicMock() + mock_content.decoded_content = b"nothing useful" + + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + assert fetch_gitreview_github_api(mock_repo) is None + + def test_missing_decoded_content_attr(self) -> None: + """Handle objects without decoded_content gracefully.""" + mock_content = object() # No decoded_content attribute at all + + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + # getattr(content, "decoded_content", b"") returns b"" + assert fetch_gitreview_github_api(mock_repo) is None + + +# ----------------------------------------------------------------------- +# fetch_gitreview — unified multi-strategy fetcher +# ----------------------------------------------------------------------- + + +class TestFetchGitreview: + def test_local_file_wins(self, tmp_path: Path) -> None: + p = tmp_path / ".gitreview" + p.write_text(TYPICAL_GITREVIEW, encoding="utf-8") + + info = fetch_gitreview(local_path=p) + assert info is not None + assert info.host == "gerrit.linuxfoundation.org" + + def test_skip_local_flag( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """When skip_local=True, local file is not read even if it exists.""" + p = tmp_path / ".gitreview" + p.write_text(TYPICAL_GITREVIEW, encoding="utf-8") + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + # No repo_obj, no repo_full → all strategies exhausted + info = fetch_gitreview(local_path=p, skip_local=True, repo_full="") + assert info is None + + def test_local_path_none_skips_local( + self, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + info = fetch_gitreview(local_path=None, repo_full="") + assert info is None + + def test_github_api_fallback(self, tmp_path: Path) -> None: + """When local file is missing, GitHub API is tried.""" + mock_content = MagicMock() + mock_content.decoded_content = MINIMAL_GITREVIEW.encode("utf-8") + + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + info = fetch_gitreview( + local_path=tmp_path / "nonexistent", + repo_obj=mock_repo, + ) + assert info is not None + assert info.host == "gerrit.example.org" + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_raw_url_fallback( + self, + mock_urlopen: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """When local and API both fail, raw URL is tried.""" + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + mock_urlopen.return_value = _mock_urlopen_for_text(MINIMAL_GITREVIEW) + + info = fetch_gitreview( + local_path=tmp_path / "nonexistent", + repo_obj=None, + repo_full="owner/repo", + include_env_refs=False, + ) + assert info is not None + assert info.host == "gerrit.example.org" + + def test_all_strategies_fail( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + info = fetch_gitreview( + local_path=tmp_path / "nonexistent", + repo_obj=None, + repo_full="", + ) + assert info is None + + def test_local_takes_priority_over_api(self, tmp_path: Path) -> None: + """Local file should win even when repo_obj is provided.""" + p = tmp_path / ".gitreview" + p.write_text( + "[gerrit]\nhost=local.example.org\nproject=local/proj.git\n", + encoding="utf-8", + ) + + mock_content = MagicMock() + mock_content.decoded_content = ( + b"[gerrit]\nhost=api.example.org\nproject=api/proj.git\n" + ) + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + info = fetch_gitreview(local_path=p, repo_obj=mock_repo) + assert info is not None + assert info.host == "local.example.org" + # API should NOT have been called + mock_repo.get_contents.assert_not_called() + + def test_api_takes_priority_over_raw( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """GitHub API should win over raw URL fallback.""" + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + mock_content = MagicMock() + mock_content.decoded_content = ( + b"[gerrit]\nhost=api.example.org\nproject=api/proj.git\n" + ) + mock_repo = MagicMock() + mock_repo.get_contents.return_value = mock_content + + with patch( + "github2gerrit.gitreview.urllib.request.urlopen" + ) as mock_urlopen: + info = fetch_gitreview( + local_path=tmp_path / "nonexistent", + repo_obj=mock_repo, + repo_full="owner/repo", + ) + + assert info is not None + assert info.host == "api.example.org" + mock_urlopen.assert_not_called() + + +# ----------------------------------------------------------------------- +# read_gitreview_host — convenience host-only accessor +# ----------------------------------------------------------------------- + + +class TestReadGitreviewHost: + def test_returns_host_from_local( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + (tmp_path / ".gitreview").write_text( + TYPICAL_GITREVIEW, encoding="utf-8" + ) + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + + result = read_gitreview_host() + assert result == "gerrit.linuxfoundation.org" + + def test_returns_none_when_unavailable( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + assert read_gitreview_host() is None + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_falls_back_to_remote( + self, + mock_urlopen: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + mock_urlopen.return_value = _mock_urlopen_for_text( + "[gerrit]\nhost=git.opendaylight.org\n" + ) + + result = read_gitreview_host("opendaylight/aaa") + assert result == "git.opendaylight.org" + + def test_uses_github_repository_env( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.setenv("GITHUB_REPOSITORY", "owner/repo") + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + with patch( + "github2gerrit.gitreview.urllib.request.urlopen" + ) as mock_urlopen: + mock_urlopen.return_value = _mock_urlopen_for_text( + MINIMAL_GITREVIEW + ) + result = read_gitreview_host() + + assert result == "gerrit.example.org" + + +# ----------------------------------------------------------------------- +# make_gitreview_info — factory with auto base_path +# ----------------------------------------------------------------------- + + +class TestMakeGitreviewInfo: + def test_basic_construction(self) -> None: + info = make_gitreview_info("gerrit.example.org", 29418, "proj") + assert info.host == "gerrit.example.org" + assert info.port == 29418 + assert info.project == "proj" + assert info.base_path is None # unknown host + + def test_auto_derives_base_path_for_known_host(self) -> None: + info = make_gitreview_info("gerrit.linuxfoundation.org", 29418, "foo") + assert info.base_path == "infra" + + def test_explicit_base_path_overrides(self) -> None: + info = make_gitreview_info( + "gerrit.linuxfoundation.org", + base_path="custom", + ) + assert info.base_path == "custom" + + def test_explicit_none_suppresses_lookup(self) -> None: + info = make_gitreview_info( + "gerrit.linuxfoundation.org", + base_path=None, + ) + assert info.base_path is None + + def test_default_port(self) -> None: + info = make_gitreview_info("h") + assert info.port == DEFAULT_GERRIT_PORT + + def test_default_project(self) -> None: + info = make_gitreview_info("h") + assert info.project == "" + + def test_result_is_frozen(self) -> None: + info = make_gitreview_info("h") + with pytest.raises(AttributeError): + info.host = "other" # type: ignore[misc] + + def test_result_is_gitreviewinfo(self) -> None: + info = make_gitreview_info("h") + assert isinstance(info, GitReviewInfo) + assert isinstance(info, GerritInfo) + + +# ----------------------------------------------------------------------- +# Integration: config.py delegation +# ----------------------------------------------------------------------- + + +class TestConfigDelegation: + """Verify config._read_gitreview_host delegates to shared module.""" + + def test_local_file( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + (tmp_path / ".gitreview").write_text( + TYPICAL_GITREVIEW, encoding="utf-8" + ) + monkeypatch.chdir(tmp_path) + + from github2gerrit.config import _read_gitreview_host + + result = _read_gitreview_host() + assert result == "gerrit.linuxfoundation.org" + + def test_none_when_missing( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + + from github2gerrit.config import _read_gitreview_host + + assert _read_gitreview_host() is None + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_remote_fallback( + self, + mock_urlopen: MagicMock, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("GITHUB_REPOSITORY", raising=False) + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + mock_urlopen.return_value = _mock_urlopen_for_text( + "[gerrit]\nhost=git.opendaylight.org\n" + ) + + from github2gerrit.config import _read_gitreview_host + + result = _read_gitreview_host("opendaylight/aaa") + assert result == "git.opendaylight.org" + + +# ----------------------------------------------------------------------- +# Integration: core.py GerritInfo re-export +# ----------------------------------------------------------------------- + + +class TestCoreReExport: + """Verify core.py re-exports GerritInfo from gitreview module.""" + + def test_import_works(self) -> None: + from github2gerrit.core import GerritInfo as CoreGerritInfo + + info = CoreGerritInfo(host="h", port=1, project="p") + assert info.host == "h" + assert info.port == 1 + assert info.project == "p" + + def test_base_path_field_available(self) -> None: + from github2gerrit.core import GerritInfo as CoreGerritInfo + + info = CoreGerritInfo(host="h", port=1, project="p", base_path="bp") + assert info.base_path == "bp" + + def test_backward_compatible_without_base_path(self) -> None: + from github2gerrit.core import GerritInfo as CoreGerritInfo + + info = CoreGerritInfo(host="h", port=1, project="p") + assert info.base_path is None + + +# ----------------------------------------------------------------------- +# Edge cases and regression tests +# ----------------------------------------------------------------------- + + +class TestEdgeCases: + """Edge cases and regressions from the original implementations.""" + + def test_parse_handles_windows_line_endings(self) -> None: + text = "[gerrit]\r\nhost=gerrit.example.org\r\nport=29418\r\n" + info = parse_gitreview(text) + assert info is not None + # .strip() in the parser should handle trailing \r + assert info.host == "gerrit.example.org" + + def test_parse_handles_trailing_whitespace(self) -> None: + text = "[gerrit]\nhost=gerrit.example.org \nport=29418\n" + info = parse_gitreview(text) + assert info is not None + assert info.host == "gerrit.example.org" + + def test_parse_captures_inline_comments_as_value(self) -> None: + """INI-style comments after values are NOT standard for .gitreview. + The parser should capture the full line after '=' and strip it, + so an inline comment becomes part of the value.""" + text = "[gerrit]\nhost=gerrit.example.org # primary\nport=29418\n" + info = parse_gitreview(text) + assert info is not None + assert info.host == "gerrit.example.org # primary" + + @patch("github2gerrit.gitreview.urllib.request.urlopen") + def test_raw_fetch_with_refs_heads_prefix( + self, + mock_urlopen: MagicMock, + monkeypatch: pytest.MonkeyPatch, + ) -> None: + """Verify the URL includes refs/heads/ prefix.""" + monkeypatch.delenv("GITHUB_HEAD_REF", raising=False) + monkeypatch.delenv("GITHUB_BASE_REF", raising=False) + mock_urlopen.return_value = _mock_urlopen_for_text(MINIMAL_GITREVIEW) + + fetch_gitreview_raw("owner/repo", include_env_refs=False) + url = mock_urlopen.call_args[0][0] + assert "/refs/heads/master/.gitreview" in url + + def test_parse_multiple_sections_takes_first_host(self) -> None: + """If there are duplicate host= lines, take the first one (regex behaviour).""" + text = "[gerrit]\nhost=first.example.org\nhost=second.example.org\n" + info = parse_gitreview(text) + assert info is not None + assert info.host == "first.example.org" + + def test_make_gitreview_info_known_host_gets_base_path(self) -> None: + """Regression: _resolve_gerrit_info in core.py should auto-derive base_path.""" + info = make_gitreview_info( + host="gerrit.linuxfoundation.org", + port=29418, + project="releng/builder", + ) + assert info.base_path == "infra" + + def test_gitreview_info_is_hashable(self) -> None: + """Frozen dataclasses should be hashable for use in sets/dicts.""" + info = GitReviewInfo(host="h", port=1, project="p") + assert hash(info) is not None + s = {info} + assert info in s diff --git a/tests/test_issue_157_regressions.py b/tests/test_issue_157_regressions.py index 00fefea..eec7b57 100644 --- a/tests/test_issue_157_regressions.py +++ b/tests/test_issue_157_regressions.py @@ -140,7 +140,7 @@ def test_reads_host_case_insensitive( class TestReadGitreviewHostRemote: """Tests for fetching .gitreview from raw.githubusercontent.com.""" - @patch("github2gerrit.config.urllib.request.urlopen") + @patch("github2gerrit.gitreview.urllib.request.urlopen") def test_fetches_from_master_branch( self, mock_urlopen: MagicMock, @@ -163,7 +163,7 @@ def test_fetches_from_master_branch( result = _read_gitreview_host("opendaylight/aaa") assert result == "git.opendaylight.org" - @patch("github2gerrit.config.urllib.request.urlopen") + @patch("github2gerrit.gitreview.urllib.request.urlopen") def test_returns_none_when_remote_unavailable( self, mock_urlopen: MagicMock, @@ -199,7 +199,7 @@ def test_returns_none_for_bare_repository_name( result = _read_gitreview_host("noslash") assert result is None - @patch("github2gerrit.config.urllib.request.urlopen") + @patch("github2gerrit.gitreview.urllib.request.urlopen") def test_prefers_github_head_ref_over_master( self, mock_urlopen: MagicMock, @@ -227,7 +227,7 @@ def test_prefers_github_head_ref_over_master( first_call_url = mock_urlopen.call_args_list[0][0][0] assert "feature/my-branch" in first_call_url - @patch("github2gerrit.config.urllib.request.urlopen") + @patch("github2gerrit.gitreview.urllib.request.urlopen") def test_tries_base_ref_after_head_ref( self, mock_urlopen: MagicMock, diff --git a/uv.lock b/uv.lock index 5ff58f9..b5386b4 100644 --- a/uv.lock +++ b/uv.lock @@ -359,7 +359,6 @@ dev = [ { name = "pytest-mock" }, { name = "responses" }, { name = "ruff" }, - { name = "types-click" }, { name = "types-requests" }, { name = "types-urllib3" }, ] @@ -387,7 +386,6 @@ requires-dist = [ { name = "rich", specifier = ">=14.2.0" }, { name = "ruff", marker = "extra == 'dev'", specifier = ">=0.6.3" }, { name = "typer", specifier = ">=0.20.1" }, - { name = "types-click", marker = "extra == 'dev'", specifier = ">=7.1.8" }, { name = "types-requests", marker = "extra == 'dev'", specifier = ">=2.31.0" }, { name = "types-urllib3", marker = "extra == 'dev'", specifier = ">=1.26.25.14" }, { name = "urllib3", specifier = ">=2.6.3" }, @@ -978,15 +976,6 @@ wheels = [ { url = "https://files.pythonhosted.org/packages/4a/91/48db081e7a63bb37284f9fbcefda7c44c277b18b0e13fbc36ea2335b71e6/typer-0.24.1-py3-none-any.whl", hash = "sha256:112c1f0ce578bfb4cab9ffdabc68f031416ebcc216536611ba21f04e9aa84c9e", size = 56085, upload-time = "2026-02-21T16:54:41.616Z" }, ] -[[package]] -name = "types-click" -version = "7.1.8" -source = { registry = "https://pypi.org/simple" } -sdist = { url = "https://files.pythonhosted.org/packages/00/ff/0e6a56108d45c80c61cdd4743312d0304d8192482aea4cce96c554aaa90d/types-click-7.1.8.tar.gz", hash = "sha256:b6604968be6401dc516311ca50708a0a28baa7a0cb840efd7412f0dbbff4e092", size = 10015, upload-time = "2021-11-23T12:28:01.701Z" } -wheels = [ - { url = "https://files.pythonhosted.org/packages/ee/ad/607454a5f991c5b3e14693a7113926758f889138371058a5f72f567fa131/types_click-7.1.8-py3-none-any.whl", hash = "sha256:8cb030a669e2e927461be9827375f83c16b8178c365852c060a34e24871e7e81", size = 12929, upload-time = "2021-11-23T12:27:59.493Z" }, -] - [[package]] name = "types-requests" version = "2.32.4.20260107" From 06c6da8dbd7820decbe02018d893df8f9b9740d3 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Mon, 16 Mar 2026 14:12:26 +0000 Subject: [PATCH 8/9] Chore: Switch from pre-commit to prek prek is a faster, Rust-based drop-in replacement that reads the existing .pre-commit-config.yaml unchanged. Install with: uv tool install prek && prek install -f Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- README.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 410f5e4..9df58f2 100644 --- a/README.md +++ b/README.md @@ -1591,7 +1591,9 @@ requiring manual configuration per PR or user. - `src/github2gerrit/gitutils.py` (subprocess and git helpers) - Linting and type checking - Ruff and MyPy use settings in `pyproject.toml`. - - Run from pre‑commit hooks and CI. + - Run from [prek](https://github.com/j178/prek) hooks and CI. + - prek is a faster, Rust-based drop-in replacement for pre-commit + that reads the existing `.pre-commit-config.yaml` unchanged. - Tests - Pytest with coverage targets around 80%. - Add unit and integration tests for each feature. @@ -1601,6 +1603,10 @@ requiring manual configuration per PR or user. - Install `uv` and run: - `uv pip install --system .` - `uv run github2gerrit --help` +- Install prek hooks: + - `uv tool install prek && prek install -f` +- Run all checks (including tests) manually: + - `prek run --all-files` - Run tests: - `uv run pytest -q` - Lint and type check: From a8ce19650d8906a99c8152d8d9683f12e9cf1145 Mon Sep 17 00:00:00 2001 From: Matthew Watkins Date: Mon, 16 Mar 2026 14:37:18 +0000 Subject: [PATCH 9/9] CI: Update release drafter configuration and workflows Use emoji in the release template to be more compatible with different platforms. Also fix the conventional commit patterns to allow for topics and also add logic for auto-versioning. Changes incorporated from lfreleng-actions/actions-template#133: - Replace emoji shortcodes with Unicode emoji - Merge Enhancements and New Features into single category - Add Performance category with 'performance' label - Add new labels: style, build, revert to Maintenance - Fix autolabeler regexes to support commit scopes - Add new autolabeler entries: style, performance, revert, build - Add version-resolver section for automatic semver - Move concurrency from job-level to workflow-level - Include event_name in concurrency group format - Refactor show-concurrency step to use GROUP variable - Add if guard for fork/same-repo PR deduplication Co-Authored-By: Claude Signed-off-by: Matthew Watkins --- .github/release-drafter.yml | 70 ++++++++++++++++++++------ .github/workflows/release-drafter.yaml | 24 +++++++-- 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/.github/release-drafter.yml b/.github/release-drafter.yml index 9a52d0e..09d7274 100644 --- a/.github/release-drafter.yml +++ b/.github/release-drafter.yml @@ -7,21 +7,22 @@ tag-template: "v$RESOLVED_VERSION" change-template: "- $TITLE @$AUTHOR (#$NUMBER)" sort-direction: ascending categories: - - title: ":boom: Breaking Change :boom:" + - title: "💥 Breaking Change 💥" labels: - "breaking-change" - - title: ":zap: Enhancements :zap:" + - title: "✨ New Features ✨" labels: - "enhancement" - - title: ":sparkles: New Features :sparkles:" - labels: - "feature" - - title: ":bug: Bug Fixes :bug:" + - title: "🐛 Bug Fixes 🐛" labels: - "fix" - "bugfix" - "bug" - - title: ":wrench: Maintenance :wrench:" + - title: "⚡ Performance ⚡" + labels: + - "performance" + - title: "🔧 Maintenance 🔧" labels: - "chore" - "documentation" @@ -30,7 +31,10 @@ categories: - "dependencies" - "github_actions" - "refactor" - - title: ":mortar_board: Code Quality :mortar_board:" + - "style" + - "build" + - "revert" + - title: "🎓 Code Quality 🎓" labels: - "code-quality" - "CI" @@ -38,28 +42,64 @@ categories: autolabeler: - label: "breaking-change" title: - - "/!:/i" + - '/^[a-z]+(\([^)]+\))?!:/i' - label: "feature" title: - - "/feat:/i" + - '/^feat(\([^)]+\))?:/i' - label: "bug" title: - - "/fix:/i" + - '/^fix(\([^)]+\))?:/i' - label: "refactor" title: - - "/refactor:/i" + - '/^refactor(\([^)]+\))?:/i' - label: "code-quality" title: - - "/test:/i" + - '/^test(\([^)]+\))?:/i' - label: "CI" title: - - "/ci:/i" + - '/^ci(\([^)]+\))?:/i' - label: "chore" title: - - "/chore:/i" + - '/^chore(\([^)]+\))?:/i' - label: "documentation" title: - - "/docs:/i" + - '/^docs(\([^)]+\))?:/i' + - label: "style" + title: + - '/^style(\([^)]+\))?:/i' + - label: "performance" + title: + - '/^perf(\([^)]+\))?:/i' + - label: "revert" + title: + - '/^revert(\([^)]+\))?:/i' + - label: "build" + title: + - '/^build(\([^)]+\))?:/i' +version-resolver: + major: + labels: + - "breaking-change" + minor: + labels: + - "feature" + - "enhancement" + patch: + labels: + - "bug" + - "fix" + - "bugfix" + - "performance" + - "refactor" + - "style" + - "build" + - "chore" + - "documentation" + - "CI" + - "test" + - "code-quality" + - "revert" + default: patch # yamllint disable rule:line-length template: | $CHANGES diff --git a/.github/workflows/release-drafter.yaml b/.github/workflows/release-drafter.yaml index 2c8922d..40faa12 100644 --- a/.github/workflows/release-drafter.yaml +++ b/.github/workflows/release-drafter.yaml @@ -26,12 +26,25 @@ permissions: {} concurrency: # yamllint disable-line rule:line-length - group: "rd-${{ github.event_name }}-${{ github.event.number }}-${{ github.ref }}" + group: ${{ github.event.pull_request.number && format('rd-{0}-pr-{1}', github.event_name, github.event.pull_request.number) || format('rd-push-{0}', github.ref) }} cancel-in-progress: true jobs: update_release_draft: name: 'Update Release Draft' + # Run on pull_request_target for forks, or pull_request for same-repo PRs + # This prevents duplicate runs for same-repo PRs + # yamllint disable rule:line-length + if: > + (github.event_name == 'pull_request_target' && github.event.pull_request.head.repo.fork) || + (github.event_name == 'pull_request' && !github.event.pull_request.head.repo.fork) || + github.event_name == 'push' + # yamllint enable rule:line-length + # SECURITY: pull_request_target with write permissions is safe here because: + # 1. This workflow does NOT checkout any code from the PR + # 2. The workflow code itself runs from the base branch (not the fork) + # 3. release-drafter only makes GitHub API calls (no code execution) + # 4. pull_request_target is needed ONLY for autolabeling fork PRs permissions: # write permission is required to create releases contents: write @@ -39,9 +52,9 @@ jobs: pull-requests: write runs-on: 'ubuntu-latest' timeout-minutes: 3 - # yamllint disable rule:line-length steps: # Harden the runner used by this workflow + # yamllint disable-line rule:line-length - uses: step-security/harden-runner@58077d3c7e43986b6b15fba718e8ea69e387dfcc # v2.15.1 with: egress-policy: 'audit' @@ -51,13 +64,16 @@ jobs: # yamllint disable rule:line-length run: | # Show concurrency group + GROUP="${{ github.event.pull_request.number && format('rd-{0}-pr-{1}', github.event_name, github.event.pull_request.number) || format('rd-push-{0}', github.ref) }}" { echo '## Release Drafter' - echo "Concurrency group: rd-${{ github.event_name }}-${{ github.event.number }}-${{ github.ref }}" + echo "Concurrency group: ${GROUP}" } >> "$GITHUB_STEP_SUMMARY" - echo "Concurrency group: rd-${{ github.event_name }}-${{ github.event.number }}-${{ github.ref }}" + echo "Concurrency group: ${GROUP}" + # yamllint enable rule:line-length - name: 'Update draft release' + # yamllint disable-line rule:line-length uses: release-drafter/release-drafter@3a7fb5c85b80b1dda66e1ccb94009adbbd32fce3 # v7.0.0 env: GITHUB_TOKEN: "${{ secrets.GITHUB_TOKEN }}"