diff --git a/.github/scripts/deploy_observability_collector.sh b/.github/scripts/deploy_observability_collector.sh index e2ec1f45d..49e8ea05c 100755 --- a/.github/scripts/deploy_observability_collector.sh +++ b/.github/scripts/deploy_observability_collector.sh @@ -140,9 +140,11 @@ if [[ -n "${public_members}" || "${invoker_iam_disabled}" == "true" ]]; then fi if [[ -n "${GITHUB_OUTPUT:-}" ]]; then - echo "revision=${revision}" >> "${GITHUB_OUTPUT}" - echo "service_url=${service_url}" >> "${GITHUB_OUTPUT}" - echo "image_uri=${image_uri}" >> "${GITHUB_OUTPUT}" + { + echo "revision=${revision}" + echo "service_url=${service_url}" + echo "image_uri=${image_uri}" + } >> "${GITHUB_OUTPUT}" fi printf 'Deployed collector revision %s at %s\n' "${revision}" "${service_url}" diff --git a/.github/scripts/require_git_ref.sh b/.github/scripts/require_git_ref.sh new file mode 100644 index 000000000..0de0bb311 --- /dev/null +++ b/.github/scripts/require_git_ref.sh @@ -0,0 +1,16 @@ +#!/usr/bin/env bash + +# Fail unless the workflow runs from the expected git ref. +# +# Usage: require_git_ref.sh +# Reads GITHUB_REF, which GitHub Actions sets for every job. + +set -euo pipefail + +expected_ref="${1:?expected git ref is required}" +message="${2:?error message is required}" + +if [[ "${GITHUB_REF:-}" != "${expected_ref}" ]]; then + echo "::error::${message}" + exit 1 +fi diff --git a/.github/workflows/alembic-v1-check.yml b/.github/workflows/alembic-v1-check.yml index 6e9094b16..43a2fef7c 100644 --- a/.github/workflows/alembic-v1-check.yml +++ b/.github/workflows/alembic-v1-check.yml @@ -33,10 +33,7 @@ jobs: - name: Install dependencies run: make install - name: Test Alembic configuration and MySQL lifecycle - run: >- - python -m pytest tests/unit/data/test_alembic_baseline.py - tests/unit/data/test_v1_database_migration.py - tests/integration/test_alembic_mysql_lifecycle.py -q + run: python -m pytest tests/unit/data/test_alembic_baseline.py tests/unit/data/test_v1_database_migration.py tests/integration/test_alembic_mysql_lifecycle.py -q env: ALEMBIC_DATABASE_URL: mysql+pymysql://root:policyengine_test@127.0.0.1:3306/policyengine_alembic_test - name: Require database at all v1 heads diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 117b1ee5e..7c2fb0cbf 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -228,8 +228,10 @@ jobs: uses: actions/setup-python@v5 with: python-version: "3.12" + - name: Update package lists + run: sudo apt-get update - name: Install system dependencies - run: sudo apt-get update && sudo apt-get install -y redis-server + run: sudo apt-get install -y redis-server - name: Start Redis run: sudo systemctl start redis-server - name: Auth diff --git a/.github/workflows/sync-cloud-run-secrets.yml b/.github/workflows/sync-cloud-run-secrets.yml index c6b46f80a..9ac877456 100644 --- a/.github/workflows/sync-cloud-run-secrets.yml +++ b/.github/workflows/sync-cloud-run-secrets.yml @@ -24,13 +24,10 @@ jobs: contents: read id-token: write steps: - - name: Require master branch - if: github.ref != 'refs/heads/master' - run: | - echo "::error::Cloud Run secret sync must run from master." - exit 1 - name: Checkout repo uses: actions/checkout@v4 + - name: Require master branch + run: bash .github/scripts/require_git_ref.sh refs/heads/master "Cloud Run secret sync must run from master." - name: GCP authentication uses: "google-github-actions/auth@v2" with: diff --git a/changelog.d/ci-scripts-exceptions.changed.md b/changelog.d/ci-scripts-exceptions.changed.md new file mode 100644 index 000000000..78ea70b32 --- /dev/null +++ b/changelog.d/ci-scripts-exceptions.changed.md @@ -0,0 +1 @@ +Turned the remaining multi-line steps outside the deploy pipeline into single-line steps or a tested script, added tests for the Cloud Run image cleanup script, and fixed a shellcheck finding in the observability collector deploy script. diff --git a/docs/generated/rules.md b/docs/generated/rules.md index 458f36e46..8663517ab 100644 --- a/docs/generated/rules.md +++ b/docs/generated/rules.md @@ -8,7 +8,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce | Tier | enforced | ratcheted | advisory | doc-only | Total | | --- | ---: | ---: | ---: | ---: | ---: | -| mechanical | 37 | 3 | 2 | 12 | 54 | +| mechanical | 39 | 1 | 2 | 12 | 54 | | metric | 0 | 0 | 0 | 0 | 0 | | judgment | 0 | 0 | 0 | 0 | 0 | @@ -64,9 +64,7 @@ Every rule that governs `policyengine-api` is listed here with how it is enforce ### Ratcheted (existing violations still tolerated) -- `api.ci.multiline-run-is-script`: 10 remaining in `rules/baselines/ci-multiline-run.txt` -- `api.ci.shell-scripts-pass-shellcheck`: 1 remaining in `rules/baselines/ci-shellcheck.txt` -- `api.ci.workflow-scripts-tested`: 1 remaining in `rules/baselines/ci-untested-scripts.txt` +- `api.ci.multiline-run-is-script`: 7 remaining in `rules/baselines/ci-multiline-run.txt` ### Exceptions and review dates @@ -328,16 +326,16 @@ flowchart LR | Rule | Statement | Tier | Status | Blocking | Enforced by | CI | Proof | | --- | --- | --- | --- | --- | --- | --- | --- | | `api.ci.multiline-run-is-script` | A GitHub Actions run: block holds one line of code and no chained commands (&&, \|\|, ;); anything more lives in a tested script under .github/scripts/, called from a single-line step. | mechanical | ratcheted | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | -| `api.ci.shell-scripts-pass-shellcheck` | Every shell script under .github/scripts/ passes shellcheck. | mechanical | ratcheted | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | -| `api.ci.workflow-scripts-tested` | Every shell script a workflow calls is run by at least one test. | mechanical | ratcheted | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | +| `api.ci.shell-scripts-pass-shellcheck` | Every shell script under .github/scripts/ passes shellcheck. | mechanical | enforced | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | +| `api.ci.workflow-scripts-tested` | Every shell script a workflow calls is run by at least one test. | mechanical | enforced | yes | quality guard `ci-scripts` | `pr.yml:quality-guards` | yes | ```mermaid flowchart LR r_api_ci_multiline_run_is_script("api.ci.multiline-run-is-script (ratcheted)") e_quality_guard_ci_scripts["quality guard ci-scripts"] j_CI_pr_yml_quality_guards["CI pr.yml:quality-guards"] - r_api_ci_shell_scripts_pass_shellcheck("api.ci.shell-scripts-pass-shellcheck (ratcheted)") - r_api_ci_workflow_scripts_tested("api.ci.workflow-scripts-tested (ratcheted)") + r_api_ci_shell_scripts_pass_shellcheck("api.ci.shell-scripts-pass-shellcheck (enforced)") + r_api_ci_workflow_scripts_tested("api.ci.workflow-scripts-tested (enforced)") r_api_ci_multiline_run_is_script --> e_quality_guard_ci_scripts e_quality_guard_ci_scripts --> j_CI_pr_yml_quality_guards r_api_ci_shell_scripts_pass_shellcheck --> e_quality_guard_ci_scripts diff --git a/rules/baselines/ci-multiline-run.txt b/rules/baselines/ci-multiline-run.txt index 6013bfefa..ec7719679 100644 --- a/rules/baselines/ci-multiline-run.txt +++ b/rules/baselines/ci-multiline-run.txt @@ -1,6 +1,4 @@ # Tolerated existing violations. This list may only shrink. -.github/workflows/alembic-v1-check.yml :: mysql-lifecycle :: Test Alembic configuration and MySQL lifecycle -.github/workflows/pr.yml :: test :: Install system dependencies .github/workflows/push.yml :: deploy-cloud-run-candidate :: Compute Cloud Run candidate metadata .github/workflows/push.yml :: deploy-cloud-run-candidate :: Restore previous Cloud Run production revision .github/workflows/push.yml :: deploy-cloud-run-staging :: Compute Cloud Run staging metadata @@ -8,4 +6,3 @@ .github/workflows/push.yml :: deploy-cloud-run-staging :: Run release tests .github/workflows/push.yml :: promote-cloud-run-staging :: Restore previous Cloud Run staging revision .github/workflows/push.yml :: versioning :: Build changelog -.github/workflows/sync-cloud-run-secrets.yml :: sync-cloud-run-secrets :: Require master branch diff --git a/rules/baselines/ci-shellcheck.txt b/rules/baselines/ci-shellcheck.txt index de7e00a3d..924c0983a 100644 --- a/rules/baselines/ci-shellcheck.txt +++ b/rules/baselines/ci-shellcheck.txt @@ -1,2 +1 @@ # Tolerated existing violations. This list may only shrink. -.github/scripts/deploy_observability_collector.sh diff --git a/rules/baselines/ci-untested-scripts.txt b/rules/baselines/ci-untested-scripts.txt index b2651e812..924c0983a 100644 --- a/rules/baselines/ci-untested-scripts.txt +++ b/rules/baselines/ci-untested-scripts.txt @@ -1,2 +1 @@ # Tolerated existing violations. This list may only shrink. -.github/scripts/cleanup_cloud_run_images.sh diff --git a/rules/registry.toml b/rules/registry.toml index 1a1994188..e53acc471 100644 --- a/rules/registry.toml +++ b/rules/registry.toml @@ -771,7 +771,7 @@ job = "quality-guards" invocation = "run_quality_guards.py" [rule.baseline] file = "rules/baselines/ci-multiline-run.txt" -count = 10 +count = 7 [rule.proof] bad_fixture = "scripts/rules/tests/fixtures/ci_scripts/multiline_bad" good_fixture = "scripts/rules/tests/fixtures/ci_scripts/multiline_good" @@ -787,7 +787,7 @@ statement = "Every shell script a workflow calls is run by at least one test." rationale = "docs/engineering/skills/ci-workflows.md#testing-a-workflow-script" scope = [".github/scripts/*.sh"] tier = "mechanical" -status = "ratcheted" +status = "enforced" autofix = false [rule.enforcer] kind = "quality-guard" @@ -796,9 +796,6 @@ name = "ci-scripts" workflow = ".github/workflows/pr.yml" job = "quality-guards" invocation = "run_quality_guards.py" -[rule.baseline] -file = "rules/baselines/ci-untested-scripts.txt" -count = 1 [rule.proof] bad_fixture = "scripts/rules/tests/fixtures/ci_scripts/untested_bad" good_fixture = "scripts/rules/tests/fixtures/ci_scripts/untested_good" @@ -814,7 +811,7 @@ statement = "Every shell script under .github/scripts/ passes shellcheck." rationale = "docs/engineering/skills/ci-workflows.md#writing-a-workflow-script" scope = [".github/scripts/*.sh"] tier = "mechanical" -status = "ratcheted" +status = "enforced" autofix = false [rule.enforcer] kind = "quality-guard" @@ -823,9 +820,6 @@ name = "ci-scripts" workflow = ".github/workflows/pr.yml" job = "quality-guards" invocation = "run_quality_guards.py" -[rule.baseline] -file = "rules/baselines/ci-shellcheck.txt" -count = 1 [rule.proof] bad_fixture = "scripts/rules/tests/fixtures/ci_scripts/shellcheck/bad_script.sh" good_fixture = "scripts/rules/tests/fixtures/ci_scripts/shellcheck/good_script.sh" diff --git a/scripts/rules/ci_scripts.py b/scripts/rules/ci_scripts.py index 02b5f3f47..5a0a6a066 100644 --- a/scripts/rules/ci_scripts.py +++ b/scripts/rules/ci_scripts.py @@ -25,6 +25,13 @@ REPO_ROOT = Path(__file__).resolve().parents[2] SEPARATOR = " :: " +# Rule ids end with these names (``.ci.``). Used to attribute a +# finding to its rule once the rule is enforced and has no baseline left. +RULE_NAMES = { + "multiline": "multiline-run-is-script", + "untested": "workflow-scripts-tested", + "shellcheck": "shell-scripts-pass-shellcheck", +} @dataclass @@ -118,10 +125,15 @@ def _baseline(root: Path, path: str) -> set[str]: } -def _rule_for(registry: Registry, baseline: str) -> str | None: +def _rule_for(registry: Registry, baseline: str, check: str) -> str | None: for rule in registry.rules: if rule.baseline is not None and rule.baseline.file == baseline: return rule.id + for rule in registry.rules: + if rule.enforcer.get("name") == "ci-scripts" and rule.id.endswith( + f".ci.{RULE_NAMES[check]}" + ): + return rule.id return None @@ -130,6 +142,7 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio assert config is not None checks = ( ( + "multiline", "multi-line run: block", findings.multiline, config.multiline_baseline, @@ -137,12 +150,14 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio f"{config.scripts_dir} and call it from a single-line run:", ), ( + "untested", "untested workflow script", findings.untested, config.untested_baseline, "add a test that runs the script (see docs/engineering/skills/ci-workflows.md)", ), ( + "shellcheck", "shellcheck finding in", findings.shellcheck, config.shellcheck_baseline, @@ -150,10 +165,10 @@ def compare(registry: Registry, findings: Findings, root: Path) -> list[Violatio ), ) violations = [] - for label, found, baseline_path, fix in checks: + for check, label, found, baseline_path, fix in checks: if found is None: continue - rule_id = _rule_for(registry, baseline_path) + rule_id = _rule_for(registry, baseline_path, check) baseline = _baseline(root, baseline_path) for entry in sorted(found - baseline): violations.append(Violation(rule_id, "ci-scripts", f"{label} {entry}", fix)) @@ -199,7 +214,9 @@ def run_check(root: Path = REPO_ROOT) -> tuple[list[Violation], list[str]]: if findings.shellcheck is None and os.environ.get("GITHUB_ACTIONS"): violations.append( Violation( - _rule_for(registry, registry.ci_scripts.shellcheck_baseline), + _rule_for( + registry, registry.ci_scripts.shellcheck_baseline, "shellcheck" + ), "ci-scripts", "shellcheck is not installed in this CI job", "install it in the job (pip install shellcheck-py==)", diff --git a/scripts/rules/tests/test_ci_scripts.py b/scripts/rules/tests/test_ci_scripts.py index 13aef4fa3..c05a4bc3f 100644 --- a/scripts/rules/tests/test_ci_scripts.py +++ b/scripts/rules/tests/test_ci_scripts.py @@ -188,6 +188,22 @@ def test_new_finding_fails_with_its_rule_id(self) -> None: messages[0], ) + def test_enforced_rule_without_baseline_keeps_its_id(self) -> None: + findings = Findings(multiline=set(), untested={"x.sh"}, shellcheck=set()) + registry, _ = load_text( + REGISTRY.replace("ex.ci.multiline", "ex.ci.workflow-scripts-tested") + .replace('status = "ratcheted"', 'status = "enforced"') + .replace( + '[rule.baseline]\nfile = "rules/baselines/ci-multiline-run.txt"\ncount = 1\n', + "", + ) + ) + assert registry is not None + messages = [str(v) for v in compare(registry, findings, self.root)] + self.assertTrue( + messages[-1].startswith("[ex.ci.workflow-scripts-tested]"), messages + ) + def test_fixed_finding_must_leave_the_baseline(self) -> None: messages = self.messages(set()) self.assertIn( diff --git a/tests/unit/test_cloud_run_deploy_scripts.py b/tests/unit/test_cloud_run_deploy_scripts.py index a22289af8..2153ec036 100644 --- a/tests/unit/test_cloud_run_deploy_scripts.py +++ b/tests/unit/test_cloud_run_deploy_scripts.py @@ -2252,8 +2252,16 @@ def test_sync_cloud_run_secrets_workflow_is_manual_and_environment_gated(): assert "- production" in workflow assert "environment: ${{ inputs.deployment_environment }}" in workflow assert "id-token: write" in workflow - assert "github.ref != 'refs/heads/master'" in workflow - assert "google-github-actions/auth@v2" in workflow + checkout_step = "uses: actions/checkout@v4" + master_gate = "bash .github/scripts/require_git_ref.sh refs/heads/master" + gcp_authentication_step = "google-github-actions/auth@v2" + assert checkout_step in workflow + assert master_gate in workflow + assert gcp_authentication_step in workflow + # The gate runs a script from the repository, so it needs the checkout + # first, and it must pass before anything authenticates to GCP. + assert workflow.index(checkout_step) < workflow.index(master_gate) + assert workflow.index(master_gate) < workflow.index(gcp_authentication_step) assert ( 'workload_identity_provider: "${{ secrets.GCP_WORKLOAD_IDENTITY_PROVIDER }}"' in workflow diff --git a/tests/unit/test_workflow_guard_scripts.py b/tests/unit/test_workflow_guard_scripts.py new file mode 100644 index 000000000..9283cc15b --- /dev/null +++ b/tests/unit/test_workflow_guard_scripts.py @@ -0,0 +1,125 @@ +"""Tests for small workflow scripts: require_git_ref.sh and cleanup_cloud_run_images.sh.""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +REPO = Path(__file__).resolve().parents[2] +REQUIRE_GIT_REF = ".github/scripts/require_git_ref.sh" +CLEANUP_IMAGES = ".github/scripts/cleanup_cloud_run_images.sh" + + +def _run( + script: str, env: dict[str, str], *args: str +) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["bash", script, *args], + cwd=REPO, + env={"HOME": os.environ.get("HOME", ""), "PATH": os.environ["PATH"], **env}, + text=True, + capture_output=True, + check=False, + ) + + +def test_require_git_ref_passes_on_the_expected_ref() -> None: + result = _run( + REQUIRE_GIT_REF, + {"GITHUB_REF": "refs/heads/master"}, + "refs/heads/master", + "must run from master.", + ) + + assert result.returncode == 0, result.stderr + assert result.stdout == "" + + +def test_require_git_ref_fails_with_an_annotation_on_another_ref() -> None: + result = _run( + REQUIRE_GIT_REF, + {"GITHUB_REF": "refs/heads/feature"}, + "refs/heads/master", + "Cloud Run secret sync must run from master.", + ) + + assert result.returncode == 1 + assert result.stdout == "::error::Cloud Run secret sync must run from master.\n" + + +def _fake_gcloud(tmp_path: Path, rows: list[str], *, delete_exit: int = 0) -> Path: + """A gcloud that lists ``rows`` and records every other call.""" + bin_dir = tmp_path / "bin" + bin_dir.mkdir() + listing = tmp_path / "listing.txt" + listing.write_text("".join(f"{row}\n" for row in rows)) + calls = tmp_path / "calls.log" + gcloud = bin_dir / "gcloud" + gcloud.write_text( + "#!/usr/bin/env bash\n" + 'if [[ "$1 $2 $3 $4" == "artifacts docker images list" ]]; then\n' + f' cat "{listing}"\n' + " exit 0\n" + "fi\n" + f'echo "$*" >> "{calls}"\n' + f"exit {delete_exit}\n" + ) + gcloud.chmod(0o755) + return bin_dir + + +IMAGES = [ + "2026-01-01T00:00:00Z|repo/api|sha256:oldest", + "2026-03-01T00:00:00Z|repo/api|sha256:newest", + "2026-02-01T00:00:00Z|repo/api|sha256:middle", +] + + +def _cleanup( + tmp_path: Path, rows: list[str], **env: str +) -> tuple[subprocess.CompletedProcess[str], list[str]]: + bin_dir = _fake_gcloud(tmp_path, rows, delete_exit=int(env.pop("DELETE_EXIT", "0"))) + result = _run( + CLEANUP_IMAGES, + {"PATH": f"{bin_dir}{os.pathsep}{os.environ['PATH']}", **env}, + ) + calls_file = tmp_path / "calls.log" + calls = calls_file.read_text().splitlines() if calls_file.exists() else [] + return result, calls + + +def test_cleanup_deletes_all_but_the_newest_images(tmp_path: Path) -> None: + result, calls = _cleanup(tmp_path, IMAGES, KEEP="1") + + assert result.returncode == 0, result.stderr + assert calls == [ + "artifacts docker images delete repo/api@sha256:middle --delete-tags --quiet", + "artifacts docker images delete repo/api@sha256:oldest --delete-tags --quiet", + ] + assert "Deleted 2/2 image(s)." in result.stdout + + +def test_cleanup_dry_run_deletes_nothing(tmp_path: Path) -> None: + result, calls = _cleanup(tmp_path, IMAGES, KEEP="1", DRY_RUN="1") + + assert result.returncode == 0, result.stderr + assert calls == [] + assert "[dry-run] would delete repo/api@sha256:middle" in result.stdout + assert "Deleted" not in result.stdout + + +def test_cleanup_keeps_everything_within_the_limit(tmp_path: Path) -> None: + result, calls = _cleanup(tmp_path, IMAGES) + + assert result.returncode == 0, result.stderr + assert calls == [] + assert "Deleting: 0" in result.stdout + + +def test_cleanup_counts_only_successful_deletions(tmp_path: Path) -> None: + result, calls = _cleanup(tmp_path, IMAGES, KEEP="1", DELETE_EXIT="1") + + assert result.returncode == 0, result.stderr + assert len(calls) == 2 + assert "Deleted 0/2 image(s)." in result.stdout