From b8ab4f854b6b3481976adaf58f489c91845e6f98 Mon Sep 17 00:00:00 2001 From: emil Date: Mon, 22 Jun 2026 06:32:17 +0000 Subject: [PATCH] GRM-53: refactor: enforce script separation and document import rules --- .gitea/workflows/ci.yml | 8 +++-- AGENTS.md | 55 ++++++++++++++++++++++++++++--- docs/tech/ci-cd-workflow.md | 19 +++++++++++ scripts/ci/classify_changes.py | 6 +++- scripts/ci/distribute_molecule.py | 11 +------ scripts/ci/doc_coverage.py | 3 ++ scripts/ci/platforms.py | 22 +++++++++++++ scripts/ci/release.py | 2 +- scripts/molecule_all.py | 2 +- tests/unit/test_doc_coverage.py | 3 +- tests/unit/test_platforms.py | 25 ++++++++++++++ 11 files changed, 135 insertions(+), 21 deletions(-) create mode 100644 scripts/ci/platforms.py create mode 100644 tests/unit/test_platforms.py diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 5df2db1..39476ac 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -27,9 +27,11 @@ jobs: . .venv/bin/activate python3 scripts/check_test_speed.py --max-seconds 10 - name: Documentation coverage check + env: + PYTHONPATH: src run: | . .venv/bin/activate - PYTHONPATH=src python3 scripts/ci/doc_coverage.py + python3 scripts/ci/doc_coverage.py release-dry-run: needs: [quality, detect-changes] @@ -43,10 +45,12 @@ jobs: - name: Set up environment run: make setup - name: Release dry-run validation + env: + PYTHONPATH: . run: | . .venv/bin/activate export PATH="$HOME/.local/bin:$PATH" - PYTHONPATH=. python3 scripts/ci/release.py --dry-run || true + python3 scripts/ci/release.py --dry-run || true detect-changes: runs-on: docker diff --git a/AGENTS.md b/AGENTS.md index d404696..c0956f9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -237,15 +237,14 @@ types from accidentally skipping releases. - `hooks/**` — Git hooks **User-facing paths** (tool changes → release needed) — everything else: -- `src/gitea_runner_manager/**` — Python CLI source +- `src/gitea_runner_manager/**` — Python CLI source (except `__init__.py` and `api_clients.py`) - `ansible/**` — Ansible role - `pyproject.toml` — Package metadata -- `scripts/check_test_speed.py`, `scripts/configure_repo.py`, `scripts/install_checkmake.py` — Dev tools - Any new file type not in the allowlist **Script directory structure:** -- `scripts/` — Dev tools (run locally by developers): `check_test_speed.py`, `configure_repo.py`, `install_checkmake.py`, `install_tools.py`, `setup.py`, `molecule_all.py` -- `scripts/ci/` — CI/CD automation (run by workflows): `release.py`, `publish.py`, `auto_merge.py`, `classify_changes.py`, `detect_release_commit.py`, `push_badges.py`, `doc_coverage.py`, `sync_wiki.py`, etc. +- `scripts/` — Dev tools (run locally by developers): `check_test_speed.py`, `configure_repo.py`, `install_checkmake.py`, `install_tools.py`, `setup.py`, `molecule_all.py`, `generate_badges.py` +- `scripts/ci/` — CI/CD automation (run by workflows): `release.py`, `publish.py`, `auto_merge.py`, `classify_changes.py`, `detect_release_commit.py`, `push_badges.py`, `doc_coverage.py`, `sync_wiki.py`, `distribute_molecule.py`, `molecule_ci_guard.py`, `discover_runners.py`, `notify_failure.py`, `post_merge.py`, `pr_review.py`, `review_pr.py`, `validate_commit_msg.py`, `platforms.py` **CI behavior based on classification:** - **Molecule tests**: Only run when `ansible/` or `.ansible-lint` files change @@ -259,6 +258,52 @@ types from accidentally skipping releases. - The `classify_changes.py` script enforces this automatically — no manual intervention needed - When adding a new CI script, place it in `scripts/ci/`. Dev tools go in `scripts/`. +## Script Separation and Import Rules + +The codebase enforces strict separation between the GRM tool and CI/dev scripts: + +### Directory Layout + +| Directory | Purpose | Release impact | +|-----------|---------|----------------| +| `src/gitea_runner_manager/` | User-facing GRM CLI tool | Changes trigger release | +| `scripts/` | Dev tools (run locally) | Workflow-only (no release) | +| `scripts/ci/` | CI/CD automation (run by workflows) | Workflow-only (no release) | +| `ansible/` | Ansible role for runner setup | Changes trigger release | + +### Import Rules + +1. **`src/gitea_runner_manager/` NEVER imports from `scripts/`** — the tool is self-contained +2. **Scripts MAY import from `gitea_runner_manager`** — one-way dependency (scripts use the tool's API clients, config, i18n) +3. **Cross-script imports** (scripts importing from other scripts) are allowed within `scripts/ci/` but must be documented + +### PYTHONPATH Configuration + +Scripts have different import requirements. Workflows must set `PYTHONPATH` accordingly: + +| PYTHONPATH | When to use | Example scripts | +|------------|-------------|-----------------| +| `src` | Script imports from `gitea_runner_manager` | `auto_merge.py`, `pr_review.py`, `notify_failure.py`, `sync_wiki.py`, `post_merge.py`, `publish.py`, `classify_changes.py`, `discover_runners.py`, `doc_coverage.py` | +| `.` | Script imports from other `scripts.ci.*` modules | `release.py` (imports `classify_changes.has_user_facing_changes`) | +| (none) | Script has no GRM or cross-script imports | `detect_release_commit.py`, `distribute_molecule.py`, `molecule_ci_guard.py`, `push_badges.py`, `validate_commit_msg.py` | + +**In workflows**, always use `env:` blocks (not inline `PYTHONPATH=value`): +```yaml +- name: Run script + env: + PYTHONPATH: src + run: python3 scripts/ci/example.py +``` + +**Locally**, the current directory is in `sys.path` by default, so `PYTHONPATH` is usually not needed. + +### Shared Constants + +`scripts/ci/platforms.py` is the single source of truth for the molecule +platform matrix. Both `scripts/ci/distribute_molecule.py` (CI) and +`scripts/molecule_all.py` (dev tool) import `PLATFORMS` from it — this +avoids dev tools importing directly from CI scripts. + 2. **Publish workflow** (`.gitea/workflows/publish.yml`): - Triggers on tag push (`v*`) - Validates `PYPI_TOKEN` is set (warns if missing) @@ -319,7 +364,7 @@ main.yml → systemd_check → user_setup → rootless_docker → install_runner 6 scenarios: `default`, `multi-instance`, `lifecycle`, `template-content`, `deregister`, `update` 4 platforms: `ubuntu-2204`, `ubuntu-2404`, `debian-12`, `archlinux` -Platform list is defined in `scripts/ci/distribute_molecule.py` (single source of truth) +Platform list is defined in `scripts/ci/platforms.py` (single source of truth) ## Known Issues diff --git a/docs/tech/ci-cd-workflow.md b/docs/tech/ci-cd-workflow.md index 38bb463..6915b66 100644 --- a/docs/tech/ci-cd-workflow.md +++ b/docs/tech/ci-cd-workflow.md @@ -221,6 +221,25 @@ When adding or removing Gitea runners: 2. If runners are at the instance level, update the `MOLECULE_RUNNERS` repo variable 3. The workflow automatically scales the matrix to match available runners +### Molecule Test Distribution + +`scripts/ci/distribute_molecule.py` discovers all molecule scenarios +under `ansible/roles/*/molecule/` and crosses them with the supported +OS platform matrix (defined in `scripts/ci/platforms.py`), then splits +the resulting test pairs evenly across the requested number of runners. +Each pair is encoded as `scenario|platform_name|platform_image|platform_command`. + +`scripts/ci/molecule_ci_guard.py` runs the actual molecule test for a +given test pair, with CI context (Gitea URL, token, run ID) for +reporting results back to the commit status API. + +### Commit Message Validation + +`scripts/ci/validate_commit_msg.py` validates that commit messages +follow the conventional commit format (`feat:`, `fix:`, `docs:`, etc.). +It is used by the pre-commit hook to enforce conventional commits on +feature branches. + ### Release Commit Detection The `detect-type` job in the post-merge workflow runs diff --git a/scripts/ci/classify_changes.py b/scripts/ci/classify_changes.py index 2a62e8f..24c8927 100644 --- a/scripts/ci/classify_changes.py +++ b/scripts/ci/classify_changes.py @@ -154,7 +154,11 @@ def classify_changes(files: list[str]) -> dict[str, list[str]]: def has_user_facing_changes(base: str, head: str) -> bool: - """Check if any user-facing files changed between base and head.""" + """Check if any user-facing files changed between base and head. + + Imported by ``scripts/ci/release.py`` to decide whether a release + is needed. This is a cross-CI import that requires ``PYTHONPATH=.``. + """ files = get_changed_files(base, head) return any(is_user_facing(f) for f in files) diff --git a/scripts/ci/distribute_molecule.py b/scripts/ci/distribute_molecule.py index fa2c226..21c774e 100644 --- a/scripts/ci/distribute_molecule.py +++ b/scripts/ci/distribute_molecule.py @@ -25,20 +25,11 @@ from pathlib import Path import click from gitea_runner_manager.i18n import _ +from scripts.ci.platforms import PLATFORMS DEFAULT_MAX_RUNNERS = 3 MOLECULE_ROOT = Path("ansible/roles/gitea-runner/molecule") -#: Supported OS platform matrix. -#: Each entry maps a short name to (image, command). -#: The command must be systemd since rootless Docker requires loginctl/systemctl --user. -PLATFORMS: list[dict[str, str]] = [ - {"name": "ubuntu-2204", "image": "geerlingguy/docker-ubuntu2204-ansible:latest", "command": "/lib/systemd/systemd"}, - {"name": "ubuntu-2404", "image": "geerlingguy/docker-ubuntu2404-ansible:latest", "command": "/lib/systemd/systemd"}, - {"name": "debian-12", "image": "geerlingguy/docker-debian12-ansible:latest", "command": "/lib/systemd/systemd"}, - {"name": "archlinux", "image": "marcstraube/archlinux-ansible:latest", "command": "/usr/lib/systemd/systemd"}, -] - @dataclass(frozen=True) class TestPair: diff --git a/scripts/ci/doc_coverage.py b/scripts/ci/doc_coverage.py index 2c0054d..3aefcf4 100644 --- a/scripts/ci/doc_coverage.py +++ b/scripts/ci/doc_coverage.py @@ -47,6 +47,9 @@ REQUIRED_SCRIPTS = [ "discover_runners.py", "detect_release_commit.py", "push_badges.py", + "distribute_molecule.py", + "molecule_ci_guard.py", + "validate_commit_msg.py", ] diff --git a/scripts/ci/platforms.py b/scripts/ci/platforms.py new file mode 100644 index 0000000..4ae3beb --- /dev/null +++ b/scripts/ci/platforms.py @@ -0,0 +1,22 @@ +"""Supported OS platform matrix for molecule tests. + +Single source of truth for the platform list used by both: +- ``scripts/ci/distribute_molecule.py`` (CI parallel matrix) +- ``scripts/molecule_all.py`` (local sequential runner) + +Keeping this in a dedicated module avoids cross-imports between +dev tools and CI scripts. +""" + +from __future__ import annotations + +#: Supported OS platform matrix. +#: Each entry maps a short name to (image, command). +#: The command must be systemd since rootless Docker requires +#: loginctl/systemctl --user. +PLATFORMS: list[dict[str, str]] = [ + {"name": "ubuntu-2204", "image": "geerlingguy/docker-ubuntu2204-ansible:latest", "command": "/lib/systemd/systemd"}, + {"name": "ubuntu-2404", "image": "geerlingguy/docker-ubuntu2404-ansible:latest", "command": "/lib/systemd/systemd"}, + {"name": "debian-12", "image": "geerlingguy/docker-debian12-ansible:latest", "command": "/lib/systemd/systemd"}, + {"name": "archlinux", "image": "marcstraube/archlinux-ansible:latest", "command": "/usr/lib/systemd/systemd"}, +] diff --git a/scripts/ci/release.py b/scripts/ci/release.py index 1158395..138165e 100644 --- a/scripts/ci/release.py +++ b/scripts/ci/release.py @@ -36,7 +36,7 @@ import click from dotenv import load_dotenv # pyright: ignore[reportMissingImports,reportUnknownVariableType] from gitea_runner_manager.i18n import _ -from scripts.ci.classify_changes import has_user_facing_changes +from scripts.ci.classify_changes import has_user_facing_changes # cross-CI import, needs PYTHONPATH=. load_dotenv(override=True) diff --git a/scripts/molecule_all.py b/scripts/molecule_all.py index c397c43..4f9a175 100644 --- a/scripts/molecule_all.py +++ b/scripts/molecule_all.py @@ -19,7 +19,7 @@ from pathlib import Path import click -from scripts.ci.distribute_molecule import PLATFORMS +from scripts.ci.platforms import PLATFORMS ROLE_DIR = Path("ansible/roles/gitea-runner") SCENARIOS = ["default", "multi-instance", "lifecycle", "template-content", "deregister", "update"] diff --git a/tests/unit/test_doc_coverage.py b/tests/unit/test_doc_coverage.py index d524ab8..effd122 100644 --- a/tests/unit/test_doc_coverage.py +++ b/tests/unit/test_doc_coverage.py @@ -78,7 +78,8 @@ class TestMain: (docs / "tech" / "ci-cd-workflow.md").write_text( "auto_merge.py release.py publish.py review_pr.py " "notify_failure.py post_merge.py classify_changes.py discover_runners.py " - "detect_release_commit.py push_badges.py" + "detect_release_commit.py push_badges.py " + "distribute_molecule.py molecule_ci_guard.py validate_commit_msg.py" ) runner = CliRunner() result = runner.invoke(main, ["--docs-dir", str(docs)]) diff --git a/tests/unit/test_platforms.py b/tests/unit/test_platforms.py new file mode 100644 index 0000000..85698e8 --- /dev/null +++ b/tests/unit/test_platforms.py @@ -0,0 +1,25 @@ +"""Unit tests for scripts/ci/platforms.py.""" + +from scripts.ci.platforms import PLATFORMS + + +class TestPlatforms: + def test_platforms_not_empty(self) -> None: + assert len(PLATFORMS) >= 4 + + def test_each_platform_has_required_keys(self) -> None: + for p in PLATFORMS: + assert "name" in p + assert "image" in p + assert "command" in p + + def test_platform_names_unique(self) -> None: + names = [p["name"] for p in PLATFORMS] + assert len(names) == len(set(names)) + + def test_known_platforms_present(self) -> None: + names = {p["name"] for p in PLATFORMS} + assert "ubuntu-2204" in names + assert "ubuntu-2404" in names + assert "debian-12" in names + assert "archlinux" in names