diff --git a/.pre-commit-hooks.yaml b/.pre-commit-hooks.yaml index 1d0a6a1..b3cea9b 100644 --- a/.pre-commit-hooks.yaml +++ b/.pre-commit-hooks.yaml @@ -29,3 +29,13 @@ - pre-commit - manual files: ^restored-packages\.txt$ +- id: check-pipeline-name-only-steps + name: check pipeline name-only steps + description: check that pipeline steps don't have only a name without uses or other details + entry: check-pipeline-name-only-steps + language: python + stages: + - pre-commit + - manual + types: + - yaml diff --git a/example.pre-commit-config.yaml b/example.pre-commit-config.yaml index e10000a..d462eb3 100644 --- a/example.pre-commit-config.yaml +++ b/example.pre-commit-config.yaml @@ -18,6 +18,8 @@ repos: (?x)^( [^/]+\.ya?ml # matches .yaml or .yml files at the top level only )$ + - id: check-pipeline-name-only-steps + files: '^[^.][^/]*\.yaml$' # matches non-hidden .yaml files at the top level only - repo: https://github.com/chainguard-dev/yam rev: 768695300c5f663012a77911eb4920c12e5ed2e5 # frozen: v0.2.26 hooks: diff --git a/pre_commit_hooks/check_pipeline_name_only_steps.py b/pre_commit_hooks/check_pipeline_name_only_steps.py new file mode 100644 index 0000000..ba65696 --- /dev/null +++ b/pre_commit_hooks/check_pipeline_name_only_steps.py @@ -0,0 +1,88 @@ +from __future__ import annotations + +import argparse +import sys +from collections.abc import Iterator +from collections.abc import Sequence +from typing import Any + +import ruamel.yaml + +yaml = ruamel.yaml.YAML(typ="safe") + + +def iter_pipelines(melange_cfg: dict[str, Any]) -> Iterator[tuple[str, list[Any]]]: + """Yield (label, steps) for every pipeline in a melange config. + + Covers the main build and test pipelines and each subpackage's build and + test pipelines. A ``pipeline:`` key with no value (YAML null) yields an + empty list, matching how melange treats it. + """ + scopes: list[tuple[str, dict[str, Any]]] = [("main", melange_cfg)] + for i, subpkg in enumerate(melange_cfg.get("subpackages") or []): + if isinstance(subpkg, dict): + scopes.append((f"subpackage '{subpkg.get('name', i)}'", subpkg)) + for label, scope in scopes: + yield f"{label} pipeline", scope.get("pipeline") or [] + test = scope.get("test") or {} + if isinstance(test, dict): + yield f"{label} test pipeline", test.get("pipeline") or [] + + +def iter_steps(steps: list[Any]) -> Iterator[dict[str, Any]]: + """Yield every dict step in *steps*, descending into nested ``pipeline:`` lists.""" + for step in steps: + if not isinstance(step, dict): + continue + yield step + yield from iter_steps(step.get("pipeline") or []) + + +def check_pipeline_steps(melange_cfg: dict[str, Any]) -> list[str]: + """Return a message for every step that consists of nothing but a ``name``. + + melange runs such a step as a no-op, so it is either a typo for ``uses:`` + or a heading that was meant to sit on the step that follows it. + """ + issues = [] + for label, steps in iter_pipelines(melange_cfg): + for step in iter_steps(steps): + if set(step) == {"name"}: + issues.append( + f"{label} step '{step['name']}' has only a name and does " + "nothing; use 'uses:' for a pipeline, or fold the name into " + "the next step", + ) + return issues + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Check that no melange pipeline step consists of only a name", + ) + parser.add_argument("filenames", nargs="*", help="Filenames to check") + args = parser.parse_args(argv) + + retval = 0 + + for filename in args.filenames: + try: + with open(filename) as f: + melange_cfg = yaml.load(f) + except Exception as e: + print(f"Error loading {filename}: {e}") + retval = 1 + continue + + if not isinstance(melange_cfg, dict): + continue + + for issue in check_pipeline_steps(melange_cfg): + print(f"{filename}: {issue}") + retval = 1 + + return retval + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/setup.cfg b/setup.cfg index b508882..264145e 100644 --- a/setup.cfg +++ b/setup.cfg @@ -22,6 +22,7 @@ python_requires = >=3.9 [options.entry_points] console_scripts = shellcheck-run-steps = pre_commit_hooks.shellcheck_run_steps:main + check-pipeline-name-only-steps = pre_commit_hooks.check_pipeline_name_only_steps:main [bdist_wheel] universal = True diff --git a/test-data/README.md b/test-data/README.md new file mode 100644 index 0000000..8926836 --- /dev/null +++ b/test-data/README.md @@ -0,0 +1,30 @@ +# Test data for pre-commit hooks + +Sample melange YAML files for exercising the hooks in this repository by hand. + +## Running a hook against a test file + +From the root of this repository: + +```bash +pre-commit try-repo . check-pipeline-name-only-steps --files test-data/pipeline-name-only-bad.yaml +``` + +`pre-commit try-repo` accepts any path to a checkout of this repository, so the +same command works from another repo by replacing `.` with that path. + +## Files + +### pipeline-name-only-bad.yaml + +- Hook: `check-pipeline-name-only-steps` +- Expected: fails with five findings, one each in the main pipeline, the main + test pipeline, a subpackage pipeline, a nested `pipeline:` inside a + subpackage step, and a subpackage test pipeline. + +### pipeline-name-only-good.yaml + +- Hook: `check-pipeline-name-only-steps` +- Expected: passes. Every step has `uses:` or `runs:` alongside any `name:`, + and the last subpackage has an empty `pipeline:` key, which melange allows + and the hook must not trip over. diff --git a/test-data/pipeline-name-only-bad.yaml b/test-data/pipeline-name-only-bad.yaml new file mode 100644 index 0000000..276c53b --- /dev/null +++ b/test-data/pipeline-name-only-bad.yaml @@ -0,0 +1,42 @@ +package: + name: test-package + version: "1.0.0" + epoch: 0 + description: "Test package with incorrectly formatted pipeline steps" + copyright: + - license: Apache-2.0 + +environment: + contents: + packages: + - busybox + +pipeline: + - name: test/go-fips-check # BAD: This should be 'uses' not 'name' + - name: "Configure build" + uses: autoconf/configure + with: + opts: --enable-shared + - uses: autoconf/make + +test: + pipeline: + - name: run-tests # BAD: Only has name, no uses + - uses: test/daemon-check-output + with: + expected_output: | + Server started + +subpackages: + - name: test-subpkg + description: "Subpackage with bad pipeline" + pipeline: + - name: bad-subpkg-step # BAD: Only has name + - uses: split/dev + - name: grouped steps + pipeline: + - name: nested-bad-step # BAD: name-only inside a nested pipeline + - runs: echo hello + test: + pipeline: + - name: subpkg-test-step # BAD: Only has name in test section diff --git a/test-data/pipeline-name-only-good.yaml b/test-data/pipeline-name-only-good.yaml new file mode 100644 index 0000000..2a73c49 --- /dev/null +++ b/test-data/pipeline-name-only-good.yaml @@ -0,0 +1,54 @@ +package: + name: test-package-good + version: "1.0.0" + epoch: 0 + description: "Test package with correctly formatted pipeline steps" + copyright: + - license: Apache-2.0 + +environment: + contents: + packages: + - busybox + - go-fips-1.22 + +pipeline: + - uses: test/go-fips-check # GOOD: Uses 'uses' instead of 'name' + - name: "Configure build" + uses: autoconf/configure # GOOD: Has both name and uses + with: + opts: --enable-shared + - uses: autoconf/make # GOOD: Just uses is fine + - name: "Run custom script" # GOOD: Has name and runs + runs: | + echo "Building package" + make install + +test: + pipeline: + - uses: test/go-fips-check # GOOD: Properly uses 'uses' + - name: "Test daemon output" + uses: test/daemon-check-output # GOOD: Has both name and uses + with: + expected_output: | + Server started + - uses: test/emptypackage # GOOD: Just uses + +subpackages: + - name: test-subpkg-good + description: "Subpackage with correct pipeline" + pipeline: + - uses: split/dev # GOOD: Uses 'uses' + - name: "Move files" + runs: | # GOOD: Has name and runs + mkdir -p ${{targets.subpkgdir}}/usr/bin + mv usr/bin/tool ${{targets.subpkgdir}}/usr/bin/ + test: + pipeline: + - uses: test/emptypackage # GOOD: Uses 'uses' in test section + - name: test-subpkg-meta + description: "Meta subpackage: an empty pipeline is allowed and must not trip the hook" + pipeline: + test: + pipeline: + - uses: test/emptypackage