From 25fe4107e688c00c096d04684adaae3e0c8681da Mon Sep 17 00:00:00 2001 From: Chris Farhood Date: Mon, 11 May 2026 22:25:45 +0000 Subject: [PATCH] fix: address QA findings on detect-pipeline workflow - Fix subdirectory matching: use prefix match for .github/* paths instead of exact dirname match (fixes .github/workflows/ not matching) - Upgrade tj-actions/changed-files from v44 to v47 (Node 24 support) - Extract detection logic into scripts/detect-pipeline.sh for testability - Add 22 automated tests in scripts/test-detect-pipeline.sh covering infra-only, plugin code, mixed, and edge cases - Add test-detection-logic CI job to run tests on every PR - Update README.md to reference v47 cc @cpfarhood Co-Authored-By: Paperclip --- .github/workflows/README.md | 2 +- .github/workflows/detect-pr-pipeline.yaml | 41 +++----- scripts/detect-pipeline.sh | 44 +++++++++ scripts/test-detect-pipeline.sh | 110 ++++++++++++++++++++++ 4 files changed, 167 insertions(+), 30 deletions(-) create mode 100755 scripts/detect-pipeline.sh create mode 100755 scripts/test-detect-pipeline.sh diff --git a/.github/workflows/README.md b/.github/workflows/README.md index c0615ec..7c883fc 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -68,7 +68,7 @@ Use `tj-actions/changed-files`: ```yaml - name: Get changed files id: changed-files - uses: tj-actions/changed-files@v44 + uses: tj-actions/changed-files@v47 with: files_separator: '\n' ``` diff --git a/.github/workflows/detect-pr-pipeline.yaml b/.github/workflows/detect-pr-pipeline.yaml index 9c96ed1..74f4cf3 100644 --- a/.github/workflows/detect-pr-pipeline.yaml +++ b/.github/workflows/detect-pr-pipeline.yaml @@ -10,6 +10,16 @@ permissions: pull-requests: write jobs: + test-detection-logic: + runs-on: runners-privilegedescalation + timeout-minutes: 2 + steps: + - name: Checkout + uses: actions/checkout@v6 + + - name: Run detection tests + run: bash scripts/test-detect-pipeline.sh + detect-pipeline: runs-on: runners-privilegedescalation timeout-minutes: 5 @@ -24,7 +34,7 @@ jobs: - name: Get changed files id: changed-files - uses: tj-actions/changed-files@v44 + uses: tj-actions/changed-files@v47 with: files_separator: '\n' @@ -34,34 +44,7 @@ jobs: echo "Changed files:" echo "${{ steps.changed-files.outputs.all_changed_files }}" - pipeline="pipeline-a" - - if [ -n "${{ steps.changed-files.outputs.all_changed_files }}" ]; then - all_infra=true - while IFS= read -r file; do - filename=$(basename "$file") - dirname=$(dirname "$file") - - if [ "$dirname" = ".github" ] || \ - [[ "$filename" == *.md ]] || \ - [[ "$filename" == .eslintrc* ]] || \ - [[ "$filename" == .prettierrc* ]] || \ - [[ "$filename" == renovate.json* ]] || \ - [[ "$filename" == .gitignore ]] || \ - [[ "$filename" == .editorconfig ]] || \ - [[ "$filename" == LICENSE ]]; then - continue - else - all_infra=false - echo "Non-infra file found: $file" - break - fi - done <<< "${{ steps.changed-files.outputs.all_changed_files }}" - - if [ "$all_infra" = true ]; then - pipeline="pipeline-b" - fi - fi + pipeline=$(echo "${{ steps.changed-files.outputs.all_changed_files }}" | bash scripts/detect-pipeline.sh) echo "pipeline-type=$pipeline" >> $GITHUB_OUTPUT echo "Detected pipeline: $pipeline" diff --git a/scripts/detect-pipeline.sh b/scripts/detect-pipeline.sh new file mode 100755 index 0000000..1b2dfc2 --- /dev/null +++ b/scripts/detect-pipeline.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +set -euo pipefail + +# Reads a newline-separated list of changed files from stdin. +# Outputs "pipeline-a" or "pipeline-b" to stdout. +# Pipeline B: all files are infra-only (config, docs, workflows). +# Pipeline A: any non-infra file present. + +detect_pipeline() { + local all_infra=true + + while IFS= read -r file; do + [ -z "$file" ] && continue + + local filename + local dir + filename=$(basename "$file") + dir=$(dirname "$file") + + if [[ "$dir" == ".github" || "$dir" == .github/* ]] || \ + [[ "$filename" == *.md ]] || \ + [[ "$filename" == .eslintrc* ]] || \ + [[ "$filename" == .prettierrc* ]] || \ + [[ "$filename" == renovate.json* ]] || \ + [[ "$filename" == .gitignore ]] || \ + [[ "$filename" == .editorconfig ]] || \ + [[ "$filename" == LICENSE ]]; then + continue + else + all_infra=false + break + fi + done + + if [ "$all_infra" = true ]; then + echo "pipeline-b" + else + echo "pipeline-a" + fi +} + +if [ "${BASH_SOURCE[0]}" = "$0" ]; then + detect_pipeline +fi diff --git a/scripts/test-detect-pipeline.sh b/scripts/test-detect-pipeline.sh new file mode 100755 index 0000000..d467645 --- /dev/null +++ b/scripts/test-detect-pipeline.sh @@ -0,0 +1,110 @@ +#!/usr/bin/env bash +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +source "$SCRIPT_DIR/detect-pipeline.sh" + +PASS=0 +FAIL=0 + +assert_eq() { + local test_name="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + echo "PASS: $test_name" + PASS=$((PASS + 1)) + else + echo "FAIL: $test_name (expected=$expected, actual=$actual)" + FAIL=$((FAIL + 1)) + fi +} + +run_detect() { + echo "$1" | detect_pipeline +} + +# --- Pipeline B cases (infra-only) --- + +assert_eq "single .github root file" "pipeline-b" \ + "$(run_detect ".github/dependabot.yml")" + +assert_eq ".github/workflows subdirectory" "pipeline-b" \ + "$(run_detect ".github/workflows/ci.yaml")" + +assert_eq "deeply nested .github path" "pipeline-b" \ + "$(run_detect ".github/workflows/reusable/build.yaml")" + +assert_eq "markdown file at root" "pipeline-b" \ + "$(run_detect "README.md")" + +assert_eq "markdown in subdirectory" "pipeline-b" \ + "$(run_detect "docs/CONTRIBUTING.md")" + +assert_eq "eslintrc config" "pipeline-b" \ + "$(run_detect ".eslintrc.json")" + +assert_eq "prettierrc config" "pipeline-b" \ + "$(run_detect ".prettierrc.yaml")" + +assert_eq "renovate config" "pipeline-b" \ + "$(run_detect "renovate.json")" + +assert_eq "renovate config5" "pipeline-b" \ + "$(run_detect "renovate.json5")" + +assert_eq "gitignore" "pipeline-b" \ + "$(run_detect ".gitignore")" + +assert_eq "editorconfig" "pipeline-b" \ + "$(run_detect ".editorconfig")" + +assert_eq "LICENSE" "pipeline-b" \ + "$(run_detect "LICENSE")" + +assert_eq "mixed infra files" "pipeline-b" \ + "$(run_detect ".github/workflows/ci.yaml +README.md +.eslintrc.json +LICENSE")" + +assert_eq "workflow + markdown combo" "pipeline-b" \ + "$(run_detect ".github/workflows/detect-pr-pipeline.yaml +.github/workflows/README.md")" + +# --- Pipeline A cases (has non-infra files) --- + +assert_eq "plugin source file" "pipeline-a" \ + "$(run_detect "headlamp-polaris-plugin/src/index.tsx")" + +assert_eq "plugin package.json" "pipeline-a" \ + "$(run_detect "headlamp-polaris-plugin/package.json")" + +assert_eq "root source file" "pipeline-a" \ + "$(run_detect "src/main.ts")" + +assert_eq "mixed infra + code" "pipeline-a" \ + "$(run_detect ".github/workflows/ci.yaml +headlamp-polaris-plugin/src/index.tsx +README.md")" + +assert_eq "single non-infra file" "pipeline-a" \ + "$(run_detect "server.js")" + +# --- Edge cases --- + +assert_eq "empty input" "pipeline-b" \ + "$(run_detect "")" + +assert_eq "root dot file (not in infra list)" "pipeline-a" \ + "$(run_detect ".env")" + +assert_eq ".github-like but not .github dir" "pipeline-a" \ + "$(run_detect ".github-backup/config.yaml")" + +# --- Summary --- + +echo "" +echo "Results: $PASS passed, $FAIL failed" + +if [ "$FAIL" -gt 0 ]; then + exit 1 +fi