From 4134b27af5ed3db0424439c2c2045300a22f25b0 Mon Sep 17 00:00:00 2001 From: Amber Arcadia Date: Wed, 13 Aug 2025 14:02:52 -0400 Subject: [PATCH 1/2] Add check-pipeline-name-only-steps hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This hook validates that pipeline steps in melange YAML files don't have only a 'name' field without 'uses' or other details. This catches a common mistake where 'name' is used instead of 'uses' for pipeline steps. The check covers: - Main pipeline steps - Test pipeline steps - Subpackage pipeline steps - Subpackage test pipeline steps Includes test data files demonstrating both valid and invalid configurations. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .pre-commit-hooks.yaml | 10 ++ .../check_pipeline_name_only_steps.py | 104 ++++++++++++++++++ setup.cfg | 1 + test-data/README.md | 29 +++++ test-data/pipeline-name-only-bad.yaml | 38 +++++++ test-data/pipeline-name-only-good.yaml | 48 ++++++++ 6 files changed, 230 insertions(+) create mode 100644 pre_commit_hooks/check_pipeline_name_only_steps.py create mode 100644 test-data/README.md create mode 100644 test-data/pipeline-name-only-bad.yaml create mode 100644 test-data/pipeline-name-only-good.yaml 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/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..c110f00 --- /dev/null +++ b/pre_commit_hooks/check_pipeline_name_only_steps.py @@ -0,0 +1,104 @@ +from __future__ import annotations + +import argparse +import sys +from collections.abc import Sequence +from typing import Any + +import ruamel.yaml + +yaml = ruamel.yaml.YAML(typ="safe") + + +def check_pipeline_steps(melange_cfg: dict[str, Any]) -> tuple[bool, list[str]]: + """ + Check if any pipeline steps have only a 'name' field without 'uses' or other details. + Returns (is_valid, list_of_issues). + """ + issues = [] + + # Check main pipeline + pipelines = melange_cfg.get("pipeline", []) + for i, step in enumerate(pipelines): + if isinstance(step, dict): + # Check if step has only 'name' and no 'uses' + if "name" in step and "uses" not in step and len(step) == 1: + step_name = step.get("name", f"step {i}") + issues.append( + f"main pipeline step '{step_name}' has only a name with no 'uses' or other details", + ) + + # Check test pipeline + test_section = melange_cfg.get("test", {}) + test_pipelines = test_section.get("pipeline", []) + for i, step in enumerate(test_pipelines): + if isinstance(step, dict): + # Check if step has only 'name' and no 'uses' + if "name" in step and "uses" not in step and len(step) == 1: + step_name = step.get("name", f"step {i}") + issues.append( + f"test pipeline step '{step_name}' has only a name with no 'uses' or other details", + ) + + # Check each subpackage + for sub_idx, subpkg in enumerate(melange_cfg.get("subpackages", [])): + subpkg_name = subpkg.get("name", f"subpackage-{sub_idx}") + + # Check subpackage pipelines + subpkg_pipelines = subpkg.get("pipeline", []) + for i, step in enumerate(subpkg_pipelines): + if isinstance(step, dict): + # Check if step has only 'name' and no 'uses' + if "name" in step and "uses" not in step and len(step) == 1: + step_name = step.get("name", f"step {i}") + issues.append( + f"subpackage '{subpkg_name}' pipeline step '{step_name}' has only a name with no 'uses' or other details", + ) + + # Check subpackage test pipelines + subpkg_test_section = subpkg.get("test", {}) + subpkg_test_pipelines = subpkg_test_section.get("pipeline", []) + for i, step in enumerate(subpkg_test_pipelines): + if isinstance(step, dict): + # Check if step has only 'name' and no 'uses' + if "name" in step and "uses" not in step and len(step) == 1: + step_name = step.get("name", f"step {i}") + issues.append( + f"subpackage '{subpkg_name}' test pipeline step '{step_name}' has only a name with no 'uses' or other details", + ) + + return len(issues) == 0, issues + + +def main(argv: Sequence[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Check that pipeline steps don't have only a name without uses or other details", + ) + 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 melange_cfg: + continue + + is_valid, issues = check_pipeline_steps(melange_cfg) + if not is_valid: + for issue in issues: + 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..ab0538c --- /dev/null +++ b/test-data/README.md @@ -0,0 +1,29 @@ +# Test Data for Pre-commit Hooks + +This directory contains sample YAML files that can be used to test the pre-commit hooks in this repository. + +## Testing hooks locally + +To test a specific hook against a test file, use the `pre-commit try-repo` command: + +```bash +# From any directory with YAML files to test: +pre-commit try-repo /path/to/this/repo HOOK_ID --files FILE_TO_TEST + +# Example for check-pipeline-name-only-steps: +pre-commit try-repo /home/amber-arcadia/Documents/GitRepos/pre-commit-hooks \ + check-pipeline-name-only-steps \ + --files test-data/pipeline-name-only-bad.yaml +``` + +## Test files + +### pipeline-name-only-bad.yaml +- **Tests**: `check-pipeline-name-only-steps` +- **Expected**: Should FAIL +- **Issues**: Contains pipeline steps that have only a `name` field without `uses` or other details + +### pipeline-name-only-good.yaml +- **Tests**: `check-pipeline-name-only-steps` +- **Expected**: Should PASS +- **Issues**: None - all pipeline steps are properly formatted \ No newline at end of file diff --git a/test-data/pipeline-name-only-bad.yaml b/test-data/pipeline-name-only-bad.yaml new file mode 100644 index 0000000..1efb093 --- /dev/null +++ b/test-data/pipeline-name-only-bad.yaml @@ -0,0 +1,38 @@ +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 + test: + pipeline: + - name: subpkg-test-step # BAD: Only has name in test section \ No newline at end of file diff --git a/test-data/pipeline-name-only-good.yaml b/test-data/pipeline-name-only-good.yaml new file mode 100644 index 0000000..1a9183f --- /dev/null +++ b/test-data/pipeline-name-only-good.yaml @@ -0,0 +1,48 @@ +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 \ No newline at end of file From ef90f3b9aa928e6d96e8ab4493121d4932839e4b Mon Sep 17 00:00:00 2001 From: Amber Arcadia Date: Fri, 2 Oct 2026 13:52:29 -0400 Subject: [PATCH 2/2] check-pipeline-name-only-steps: guard null pipelines, dedupe, document - A `pipeline:` key with no value (YAML null) crashed the hook with TypeError; five files on stereo main have one. Every pipeline and test lookup now falls back to an empty list. - The four copies of the same loop collapse into iter_pipelines(), which yields (label, steps) for the main and subpackage build and test pipelines, and iter_steps(), which also descends into nested `pipeline:` lists so a name-only step inside a grouped step is caught. - The message says what to do: use `uses:`, or fold the name into the next step. - test-data/README.md loses the hard-coded home directory path; the bad fixture gains a nested case and the good fixture a null pipeline. - The hook is listed in example.pre-commit-config.yaml. On stereo main today the hook reports 30 steps in 25 files, none of them the `- name: test/go-fips-check` typo it was written for: 3 are placeholder pipelines in meta packages and 27 are `- name:` headings written as their own list item ahead of an anonymous `- runs:` or `- uses:`. melange runs both as no-ops. Those files are fixed in stereo alongside this change. --- example.pre-commit-config.yaml | 2 + .../check_pipeline_name_only_steps.py | 102 ++++++++---------- test-data/README.md | 37 +++---- test-data/pipeline-name-only-bad.yaml | 6 +- test-data/pipeline-name-only-good.yaml | 8 +- 5 files changed, 76 insertions(+), 79 deletions(-) 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 index c110f00..ba65696 100644 --- a/pre_commit_hooks/check_pipeline_name_only_steps.py +++ b/pre_commit_hooks/check_pipeline_name_only_steps.py @@ -2,6 +2,7 @@ import argparse import sys +from collections.abc import Iterator from collections.abc import Sequence from typing import Any @@ -10,69 +11,54 @@ yaml = ruamel.yaml.YAML(typ="safe") -def check_pipeline_steps(melange_cfg: dict[str, Any]) -> tuple[bool, list[str]]: - """ - Check if any pipeline steps have only a 'name' field without 'uses' or other details. - Returns (is_valid, list_of_issues). +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. """ - issues = [] + 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 []) - # Check main pipeline - pipelines = melange_cfg.get("pipeline", []) - for i, step in enumerate(pipelines): - if isinstance(step, dict): - # Check if step has only 'name' and no 'uses' - if "name" in step and "uses" not in step and len(step) == 1: - step_name = step.get("name", f"step {i}") - issues.append( - f"main pipeline step '{step_name}' has only a name with no 'uses' or other details", - ) - # Check test pipeline - test_section = melange_cfg.get("test", {}) - test_pipelines = test_section.get("pipeline", []) - for i, step in enumerate(test_pipelines): - if isinstance(step, dict): - # Check if step has only 'name' and no 'uses' - if "name" in step and "uses" not in step and len(step) == 1: - step_name = step.get("name", f"step {i}") +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"test pipeline step '{step_name}' has only a name with no 'uses' or other details", + 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", ) - - # Check each subpackage - for sub_idx, subpkg in enumerate(melange_cfg.get("subpackages", [])): - subpkg_name = subpkg.get("name", f"subpackage-{sub_idx}") - - # Check subpackage pipelines - subpkg_pipelines = subpkg.get("pipeline", []) - for i, step in enumerate(subpkg_pipelines): - if isinstance(step, dict): - # Check if step has only 'name' and no 'uses' - if "name" in step and "uses" not in step and len(step) == 1: - step_name = step.get("name", f"step {i}") - issues.append( - f"subpackage '{subpkg_name}' pipeline step '{step_name}' has only a name with no 'uses' or other details", - ) - - # Check subpackage test pipelines - subpkg_test_section = subpkg.get("test", {}) - subpkg_test_pipelines = subpkg_test_section.get("pipeline", []) - for i, step in enumerate(subpkg_test_pipelines): - if isinstance(step, dict): - # Check if step has only 'name' and no 'uses' - if "name" in step and "uses" not in step and len(step) == 1: - step_name = step.get("name", f"step {i}") - issues.append( - f"subpackage '{subpkg_name}' test pipeline step '{step_name}' has only a name with no 'uses' or other details", - ) - - return len(issues) == 0, issues + return issues def main(argv: Sequence[str] | None = None) -> int: parser = argparse.ArgumentParser( - description="Check that pipeline steps don't have only a name without uses or other details", + 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) @@ -88,13 +74,11 @@ def main(argv: Sequence[str] | None = None) -> int: retval = 1 continue - if not melange_cfg: + if not isinstance(melange_cfg, dict): continue - is_valid, issues = check_pipeline_steps(melange_cfg) - if not is_valid: - for issue in issues: - print(f"{filename}: {issue}") + for issue in check_pipeline_steps(melange_cfg): + print(f"{filename}: {issue}") retval = 1 return retval diff --git a/test-data/README.md b/test-data/README.md index ab0538c..8926836 100644 --- a/test-data/README.md +++ b/test-data/README.md @@ -1,29 +1,30 @@ -# Test Data for Pre-commit Hooks +# Test data for pre-commit hooks -This directory contains sample YAML files that can be used to test the pre-commit hooks in this repository. +Sample melange YAML files for exercising the hooks in this repository by hand. -## Testing hooks locally +## Running a hook against a test file -To test a specific hook against a test file, use the `pre-commit try-repo` command: +From the root of this repository: ```bash -# From any directory with YAML files to test: -pre-commit try-repo /path/to/this/repo HOOK_ID --files FILE_TO_TEST - -# Example for check-pipeline-name-only-steps: -pre-commit try-repo /home/amber-arcadia/Documents/GitRepos/pre-commit-hooks \ - check-pipeline-name-only-steps \ - --files test-data/pipeline-name-only-bad.yaml +pre-commit try-repo . check-pipeline-name-only-steps --files test-data/pipeline-name-only-bad.yaml ``` -## Test files +`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 -- **Tests**: `check-pipeline-name-only-steps` -- **Expected**: Should FAIL -- **Issues**: Contains pipeline steps that have only a `name` field without `uses` or other details + +- 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 -- **Tests**: `check-pipeline-name-only-steps` -- **Expected**: Should PASS -- **Issues**: None - all pipeline steps are properly formatted \ No newline at end of file + +- 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 index 1efb093..276c53b 100644 --- a/test-data/pipeline-name-only-bad.yaml +++ b/test-data/pipeline-name-only-bad.yaml @@ -33,6 +33,10 @@ subpackages: 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 \ No newline at end of file + - 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 index 1a9183f..2a73c49 100644 --- a/test-data/pipeline-name-only-good.yaml +++ b/test-data/pipeline-name-only-good.yaml @@ -45,4 +45,10 @@ subpackages: mv usr/bin/tool ${{targets.subpkgdir}}/usr/bin/ test: pipeline: - - uses: test/emptypackage # GOOD: Uses 'uses' in test section \ No newline at end of file + - 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