Public Access
fix: create_dependency_pr clones target repo instead of editing producer checkout
This commit is contained in:
+35
-15
@@ -1,27 +1,47 @@
|
||||
# DEVX-166: Exclude docs/plans/* from PR size check
|
||||
# DEVX-166: create_dependency_pr must clone the target repo
|
||||
|
||||
## Problem
|
||||
Planning docs in `docs/plans/` are legitimately large (700+ lines) but
|
||||
fail the PR size check (max 500 lines). This blocks PRs that only add
|
||||
planning documents.
|
||||
|
||||
`create_dependency_pr` resolves the pinned-version file and runs all
|
||||
git operations in the current working directory. Producer post-merge
|
||||
workflows (grm, sso-bridge) invoke it from the *producer* checkout, so
|
||||
it searches/modifies the wrong repository: `find_pinned_version` reads
|
||||
files that do not exist there, and the git fetch/checkout/commit/push
|
||||
sequence runs in the producer clone. The failure is silent — producer
|
||||
workflows append `|| echo warning`, so a no-op looks like success.
|
||||
|
||||
## Approach
|
||||
REQ-1: Add `docs/plans/*` to `DEFAULT_EXCLUDED_PATTERNS` in
|
||||
`src/devx/ci/check_pr_size.py`
|
||||
REQ-2: Add test coverage for the new exclusion pattern
|
||||
|
||||
REQ-1: Clone the target repo (`--repo`) into a temporary directory with
|
||||
an authenticated `http.extraHeader`, then run every file lookup and git
|
||||
operation (fetch, checkout, add, commit, push) inside that clone. The
|
||||
push uses the same auth header config.
|
||||
|
||||
REQ-2: Tests mock `subprocess.run` so no real clone happens in the unit
|
||||
suite (test-isolation gate).
|
||||
|
||||
## Files Affected
|
||||
|
||||
- `src/devx/ci/create_dependency_pr.py`
|
||||
- `tests/unit/test_create_dependency_pr.py`
|
||||
|
||||
## Test Plan
|
||||
- `make pytest-cov` passes with 100% coverage
|
||||
- `make lint-all` passes
|
||||
|
||||
- Existing CLI tests keep passing with the subprocess mock in place.
|
||||
- Verify the clone command targets the `--repo` URL and that git ops
|
||||
run with `cwd=<clone>` (asserted via the mock's call list).
|
||||
|
||||
## Deploy Plan
|
||||
- Merge to master → post-merge auto-publishes new devx version
|
||||
- Infra PR #1179 picks up the fix once devx is bumped
|
||||
|
||||
Merge via auto-merge after green CI. The fix takes effect the next time
|
||||
a producer post-merge workflow invokes `create_dependency_pr`.
|
||||
|
||||
## Rollback Plan
|
||||
- Revert the merge commit
|
||||
|
||||
Revert the squash-merge commit on master; the previous (broken) CWD
|
||||
behavior returns, which is strictly worse — no state is created.
|
||||
|
||||
## Acceptance Criteria
|
||||
- [x] REQ-1: Add `docs/plans/*` to `DEFAULT_EXCLUDED_PATTERNS` in
|
||||
`src/devx/ci/check_pr_size.py`
|
||||
- [x] REQ-2: Add test coverage for the new exclusion pattern
|
||||
|
||||
- [x] REQ-1: target repo cloned to tempdir; all file/git ops run in the clone
|
||||
- [x] REQ-2: unit tests never spawn a real git subprocess
|
||||
|
||||
@@ -21,6 +21,7 @@ from __future__ import annotations
|
||||
|
||||
import re
|
||||
import subprocess # nosec B404
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
import click
|
||||
@@ -128,11 +129,23 @@ def cli(
|
||||
owner, repo_name = repo.split("/", 1)
|
||||
client = GiteaClient(GITEA_API_URL, token, owner, repo_name)
|
||||
|
||||
# Implements: REQ-1 — this tool runs from the *producer* repo's CI, so
|
||||
# every file lookup and git operation must happen inside a clone of the
|
||||
# target repo, not the producer checkout in CWD.
|
||||
workdir = Path(tempfile.mkdtemp(prefix="dep-pr-"))
|
||||
clone_url = f"{GITEA_API_URL.removesuffix('/api/v1')}/{repo}.git"
|
||||
auth_cfg = f"http.extraHeader=Authorization: token {token}"
|
||||
subprocess.run( # nosec B603 B607
|
||||
["git", "-c", auth_cfg, "clone", "--depth", "50", clone_url, str(workdir)],
|
||||
check=True,
|
||||
capture_output=True,
|
||||
)
|
||||
|
||||
# Find current pinned version
|
||||
old_version = None
|
||||
changed_file = None
|
||||
for f in [PYPROJECT_PATH, IMAGES_YML_PATH, ROLE_DEFAULTS_PATH]:
|
||||
old_version = find_pinned_version(package, f)
|
||||
old_version = find_pinned_version(package, str(workdir / f))
|
||||
if old_version:
|
||||
changed_file = f
|
||||
break
|
||||
@@ -184,17 +197,21 @@ def cli(
|
||||
else:
|
||||
raise click.ClickException(_("Failed to create branch: {error}", error=str(e))) from None
|
||||
|
||||
# Clone, update file, commit, push
|
||||
subprocess.run(["git", "fetch", "origin", f"{branch_name}"], check=False, capture_output=True) # nosec B603 B607
|
||||
subprocess.run(["git", "checkout", branch_name], check=False, capture_output=True) # nosec B603 B607
|
||||
# Check out the API-created branch inside the target clone.
|
||||
subprocess.run(["git", "fetch", "origin", f"{branch_name}"], check=False, capture_output=True, cwd=workdir) # nosec B603 B607
|
||||
subprocess.run(["git", "checkout", branch_name], check=False, capture_output=True, cwd=workdir) # nosec B603 B607
|
||||
|
||||
if not changed_file or not update_pinned_version(changed_file, package, old_version, new_version):
|
||||
if not changed_file or not update_pinned_version(str(workdir / changed_file), package, old_version, new_version):
|
||||
raise click.ClickException(_("Failed to update {file}", file=changed_file))
|
||||
|
||||
subprocess.run(["git", "add", changed_file], check=True) # nosec B603 B607
|
||||
subprocess.run(["git", "add", changed_file], check=True, cwd=workdir) # nosec B603 B607
|
||||
commit_msg = f"deps: bump {package} from {old_version} to {new_version}"
|
||||
subprocess.run(["git", "commit", "-m", commit_msg], check=True) # nosec B603 B607
|
||||
subprocess.run(["git", "push", "origin", branch_name], check=True) # nosec B603 B607
|
||||
subprocess.run(["git", "commit", "-m", commit_msg], check=True, cwd=workdir) # nosec B603 B607
|
||||
subprocess.run( # nosec B603 B607
|
||||
["git", "-c", auth_cfg, "push", "origin", branch_name],
|
||||
check=True,
|
||||
cwd=workdir,
|
||||
)
|
||||
|
||||
# Create Vikunja task for tracking
|
||||
task_title = f"Bump {package} to {new_version}"
|
||||
|
||||
@@ -4,6 +4,7 @@ from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import click
|
||||
import pytest
|
||||
from click.testing import CliRunner
|
||||
|
||||
from devx.ci.create_dependency_pr import (
|
||||
@@ -15,6 +16,13 @@ from devx.ci.create_dependency_pr import (
|
||||
)
|
||||
|
||||
|
||||
@pytest.fixture(autouse=True)
|
||||
def _mock_subprocess():
|
||||
"""Mock subprocess so CLI tests never run a real git clone."""
|
||||
with patch("devx.ci.create_dependency_pr.subprocess.run") as m:
|
||||
yield m
|
||||
|
||||
|
||||
class TestFindPinnedVersion:
|
||||
def test_finds_pip_git_pin(self, tmp_path: Path) -> None:
|
||||
content = "grm @ git+https://git.example.com/repo.git@v0.5.1"
|
||||
|
||||
Reference in New Issue
Block a user