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 }}" 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: 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 54f461a..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: @@ -1620,8 +1621,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/config.py b/src/github2gerrit/config.py index 4492177..6744699 100644 --- a/src/github2gerrit/config.py +++ b/src/github2gerrit/config.py @@ -454,11 +454,38 @@ 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. + + 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. + + 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. + """ + from .gitreview import read_gitreview_host + + return read_gitreview_host(repository) + + 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 +499,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 +511,22 @@ 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 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" + + 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 = "" diff --git a/src/github2gerrit/core.py b/src/github2gerrit/core.py index 33336ad..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) @@ -1296,7 +1299,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 +1307,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) @@ -1343,19 +1347,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 = { @@ -1850,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, @@ -1871,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, @@ -2067,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 @@ -3884,17 +3855,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) @@ -4943,7 +4915,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 @@ -5444,9 +5416,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 = [ @@ -5597,7 +5570,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/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/gerrit_pr_closer.py b/src/github2gerrit/gerrit_pr_closer.py index 305dd0f..98af08e 100644 --- a/src/github2gerrit/gerrit_pr_closer.py +++ b/src/github2gerrit/gerrit_pr_closer.py @@ -1493,17 +1493,20 @@ 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, + exc_info=True, + ) + 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( 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/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/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 new file mode 100644 index 0000000..eec7b57 --- /dev/null +++ b/tests/test_issue_157_regressions.py @@ -0,0 +1,512 @@ +# 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" + + 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 +# --------------------------------------------------------------------------- + + +class TestReadGitreviewHostRemote: + """Tests for fetching .gitreview from raw.githubusercontent.com.""" + + @patch("github2gerrit.gitreview.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.gitreview.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 + + @patch("github2gerrit.gitreview.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.gitreview.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 +# --------------------------------------------------------------------------- + + +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_rest.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_rest.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_rest.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_rest.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, tmp_path: Path + ) -> 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=tmp_path) + + # 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, tmp_path: Path + ) -> 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=tmp_path) + + 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) diff --git a/uv.lock b/uv.lock index be0361e..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" }, @@ -663,11 +661,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] @@ -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"