diff --git a/.github/workflows/api-diff.yml b/.github/workflows/api-diff.yml index 3ba967cc4..6f512106b 100644 --- a/.github/workflows/api-diff.yml +++ b/.github/workflows/api-diff.yml @@ -12,32 +12,23 @@ concurrency: cancel-in-progress: true jobs: - test-conventional-commit-logic: - runs-on: ubuntu-latest - permissions: - contents: read - steps: - - name: Checkout code - uses: actions/checkout@v4 - - - name: Run conventional commit logic unit tests - run: ./scripts/api-diff/api-diff.test.sh - api-diff: runs-on: ubuntu-latest - needs: test-conventional-commit-logic + timeout-minutes: 20 permissions: contents: read - pull-requests: write steps: - name: Checkout code - uses: actions/checkout@v4 + uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: fetch-depth: 0 - name: Make script executable run: chmod +x scripts/api-diff/api-diff.sh + - name: Test fail-closed behaviour + run: bash scripts/api-diff/api-diff.test.sh + - name: Run API diff check run: ./scripts/api-diff/api-diff.sh diff --git a/scripts/api-diff/README.md b/scripts/api-diff/README.md index 75d9e6f47..fbe8a3e04 100644 --- a/scripts/api-diff/README.md +++ b/scripts/api-diff/README.md @@ -10,24 +10,32 @@ Main script that compares OpenAPI specifications against the master branch. **Usage:** ```bash # From the repo root -./scripts/api-diff/api-diff.sh [--fail-on-breaking] [filename.yaml] +./scripts/api-diff/api-diff.sh [filename.yaml] # Check all xero*.yaml files ./scripts/api-diff/api-diff.sh # Check a single file ./scripts/api-diff/api-diff.sh xero_accounting.yaml - -# Fail on breaking changes (CI mode) -./scripts/api-diff/api-diff.sh --fail-on-breaking ``` +The script always fails on breaking changes. There is no flag to waive that, and +any unrecognised argument is rejected with exit code 2. + +**Exit Codes:** +- `0` - No breaking changes detected +- `1` - Breaking changes detected, or the check could not be completed +- `2` - Unsupported argument + **Environment Variables:** -- `OASDIFF_DOCKER_IMAGE` - Docker image to use (default: `tufin/oasdiff:latest`) +- `OASDIFF_DOCKER_IMAGE` - Docker image to use (default: oasdiff 1.28.0 pinned by image digest in `api-diff.sh`) - `BASE_BRANCH` - Branch to compare against (default: `origin/master`) +When updating oasdiff, verify the release tag and the image manifest digest from the publisher, then update both the tag and digest together. Do not replace the default with a mutable tag such as `latest`. + ### `api-diff.test.sh` -Unit tests for conventional commit breaking marker detection used in GitHub Actions. +Fail-closed tests for `api-diff.sh`. They stub `docker` on `PATH` so the script's +exit codes can be checked without running oasdiff. **Usage:** ```bash @@ -35,31 +43,32 @@ Unit tests for conventional commit breaking marker detection used in GitHub Acti ``` Tests validate that: -- Commits with `!` in the conventional commit header are correctly identified -- Commits with `BREAKING CHANGE:` footer are correctly identified -- Other commits are handled with breaking change enforcement +- A clean comparison exits `0` +- A breaking change reported by oasdiff exits `1` +- A changelog generation failure exits `1` instead of being ignored +- An unsupported argument exits `2` ## Integration -These scripts are integrated into the GitHub Actions workflow at `.github/workflows/api-diff.yml`: -- **test-conventional-commit-logic** job - Runs unit tests -- **api-diff** job - Runs API diff checks with conditional breaking change enforcement - -### Conventional Commit Breaking Markers -The API diff script automatically adjusts behavior based on commit messages: - -**Allow Breaking Changes:** -- Commit header with `!`, for example: `feat!: remove deprecated endpoint` -- Commit header with scope and `!`, for example: `feat(api)!: remove deprecated endpoint` -- Commit body/footer containing `BREAKING CHANGE: ...` -- The `--fail-on-breaking` flag is NOT passed to the script - -**Fail on Breaking Changes:** -- Commits without these conventional commit breaking markers -- The `--fail-on-breaking` flag IS passed to the script -- Build will fail if breaking changes are detected - -This keeps enforcement aligned with [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/#summary) and semantic-release expectations. +These scripts run in the GitHub Actions workflow at `.github/workflows/api-diff.yml`: +- **api-diff** job - Runs the fail-closed tests, then the API diff check + +### Fail-Closed Behaviour +The check is built so that no failure mode is reported as a pass: + +- Breaking changes always fail the build. Conventional Commit markers such as + `feat!:` in the header or a `BREAKING CHANGE:` footer no longer waive the + check, and the `--fail-on-breaking` flag has been removed. +- A spec deleted or renamed in the pull request counts as a breaking change. + Specs are enumerated from the union of the base ref and the working tree, so + removing a file cannot skip the comparison. +- A base ref that cannot be fetched or resolved aborts the run rather than + comparing against nothing. +- A changelog that cannot be generated fails the run rather than logging a + warning and continuing. + +If a breaking change is intentional, coordinate the major version bump and the +release notes. There is no in-repo way to waive the check. ## Known Limitations diff --git a/scripts/api-diff/api-diff.sh b/scripts/api-diff/api-diff.sh index b5c09823c..0f072a9cc 100755 --- a/scripts/api-diff/api-diff.sh +++ b/scripts/api-diff/api-diff.sh @@ -1,20 +1,19 @@ #!/bin/bash # Script to check API diffs using oasdiff -# Usage: ./scripts/api-diff/api-diff.sh [--fail-on-breaking] [filename.yaml] +# Usage: ./scripts/api-diff/api-diff.sh [filename.yaml] # Assumes you have Docker installed and the repo is checked out with master branch available -set -e # Exit on error -set -o pipefail # Catch errors in pipes +set -euo pipefail # Change to repo root cd "$(dirname "$0")/../.." # Configuration -DOCKER_IMAGE="${OASDIFF_DOCKER_IMAGE:-tufin/oasdiff:latest}" +DOCKER_IMAGE="${OASDIFF_DOCKER_IMAGE:-tufin/oasdiff:v1.28.0@sha256:86830f988eaafcf589acb2794ee5ab78e3300ded071d6517bf085469300cbf36}" # Detect base branch from GitHub Actions environment or fallback to local defaults -if [ -n "$GITHUB_BASE_REF" ]; then +if [ -n "${GITHUB_BASE_REF:-}" ]; then # In GitHub Actions PR, use the base ref (e.g., "master") BASE_BRANCH="${BASE_BRANCH:-origin/$GITHUB_BASE_REF}" else @@ -22,76 +21,17 @@ else BASE_BRANCH="${BASE_BRANCH:-origin/master}" fi -FAIL_ON_BREAKING=false TARGET_FILE="" -DRY_RUN=false - -detect_breaking_commit_marker() { - local commits="$1" - - # Conventional Commits breaking indicators: - # 1) An exclamation mark in the type/scope header, e.g. feat!: ... or feat(api)!: ... - # 2) A BREAKING CHANGE footer in the commit body - if echo "$commits" | grep -Eiq '^[[:space:]]*[a-z]+(\([^)]+\))?!:'; then - return 0 - fi - - if echo "$commits" | grep -Eiq 'BREAKING[ -]CHANGE:'; then - return 0 - fi - - return 1 -} - -get_commit_messages() { - # For tests and local overrides - if [ -n "$COMMIT_MESSAGES" ]; then - echo "$COMMIT_MESSAGES" - return 0 - fi - - # In GitHub Actions PRs, scan all commit subjects + bodies in the PR range. - if [ -n "$GITHUB_BASE_REF" ]; then - git log --format='%s%n%b%n----' "origin/$GITHUB_BASE_REF..HEAD" 2>/dev/null || true - return 0 - fi - - # Local default: inspect HEAD commit only. - git log -1 --format='%s%n%b' 2>/dev/null || true -} # Parse arguments for arg in "$@"; do - if [ "$arg" = "--fail-on-breaking" ]; then - FAIL_ON_BREAKING=true - elif [ "$arg" = "--dry-run" ]; then - DRY_RUN=true - elif [[ "$arg" == *.yaml ]]; then + if [[ "$arg" == *.yaml ]] && [ -z "$TARGET_FILE" ]; then TARGET_FILE="$arg" - fi -done - -# If --fail-on-breaking not explicitly set, determine based on conventional commit markers. -if [ "$FAIL_ON_BREAKING" = false ]; then - COMMIT_TEXT=$(get_commit_messages) - if detect_breaking_commit_marker "$COMMIT_TEXT"; then - echo "Detected conventional commit breaking marker ('!' in header or 'BREAKING CHANGE:' footer), allowing breaking changes" - FAIL_ON_BREAKING=false - else - echo "No conventional commit breaking marker found, failing on breaking changes" - FAIL_ON_BREAKING=true - fi -fi - -if [ "$DRY_RUN" = true ]; then - if [ "$FAIL_ON_BREAKING" = true ]; then - echo "Mode: Failing on breaking changes" else - echo "Mode: Allowing breaking changes" + echo "Error: unsupported argument '$arg'" >&2 + exit 2 fi - echo "Dry run mode, exiting after commit message check" - exit 0 -fi +done echo "Starting API diff check..." @@ -101,12 +41,18 @@ if [ ! -f "xero_accounting.yaml" ]; then exit 1 fi -# Fetch master if not already done -git fetch "${BASE_BRANCH%%/*}" "${BASE_BRANCH##*/}" 2>/dev/null || echo "Warning: Could not fetch ${BASE_BRANCH}" +# Refresh and verify the exact base. A missing or stale base must not turn this +# compatibility check into a pass. +# Strip only the remote prefix: a base such as origin/feature/prism-changes +# must fetch "feature/prism-changes", not just the last path segment. +BASE_REMOTE="${BASE_BRANCH%%/*}" +BASE_REF="${BASE_BRANCH#"$BASE_REMOTE"/}" +git fetch "$BASE_REMOTE" "$BASE_REF" +git rev-parse --verify "${BASE_BRANCH}^{commit}" >/dev/null # Create temp directory for master branch files (outside repo to avoid overlap with /current mount) TEMP_DIR=$(mktemp -d) -trap "rm -rf $TEMP_DIR" EXIT +trap 'rm -rf "$TEMP_DIR"' EXIT # Get list of xero*.yaml files (excluding any master_*.yaml files) if [ -n "$TARGET_FILE" ]; then @@ -118,8 +64,14 @@ if [ -n "$TARGET_FILE" ]; then files="$TARGET_FILE" echo "Running diff for single file: $TARGET_FILE" else - # All xero*.yaml files - files=$(ls xero*.yaml 2>/dev/null | grep -v "^master_") + # Union of the specs present in the base ref and in the working tree. + # Enumerating the working tree alone would silently skip a spec deleted or + # renamed in this PR, so the removal of an entire API would never be + # compared and the check would pass. + files=$( { + ls xero*.yaml 2>/dev/null || true + git ls-tree -r --name-only "$BASE_BRANCH" 2>/dev/null || true + } | grep -E '^xero[^/]*\.yaml$' | grep -v "^master_" | sort -u || true ) if [ -z "$files" ]; then echo "No xero*.yaml files found" exit 1 @@ -127,6 +79,7 @@ else fi BREAKING_CHANGES_FOUND=false +EXECUTION_FAILED=false FILES_WITH_BREAKING_CHANGES=() TOTAL_FILES=0 PROCESSED_FILES=0 @@ -142,18 +95,26 @@ for file in $files; do echo "" echo "========== $file ==========" - # Get the file from master branch - if ! git show "$BASE_BRANCH:$file" > "$TEMP_DIR/$file" 2>/dev/null; then - echo "ℹ️ New file (does not exist in master branch)" + # A spec present in the base but absent at head removes the entire API + # surface it described. That is the most breaking change possible and must + # never be reported as a pass. + if [ ! -f "$file" ]; then + echo "❌ Spec removed (present in $BASE_BRANCH, absent at HEAD)" + BREAKING_CHANGES_FOUND=true + FILES_WITH_BREAKING_CHANGES+=("$file") + PROCESSED_FILES=$((PROCESSED_FILES + 1)) continue fi - # Verify the temp file was created - if [ ! -f "$TEMP_DIR/$file" ]; then - echo "❌ Failed to create temp file" + # Get the file from master branch + if ! git cat-file -e "$BASE_BRANCH:$file" 2>/dev/null; then + echo "ℹ️ New file (does not exist in master branch)" + PROCESSED_FILES=$((PROCESSED_FILES + 1)) continue fi + git show "$BASE_BRANCH:$file" > "$TEMP_DIR/$file" + # Note: oasdiff has some non-deterministic behavior in change counts due to # unordered map iteration in Go. Error counts are consistent, but warning # counts may vary by ~2-3% between runs. This is a known limitation. @@ -170,7 +131,8 @@ for file in $files; do if [ $CHANGELOG_EXIT -eq 0 ]; then echo "✓ Changelog generated successfully" else - echo "⚠ Could not generate changelog (exit code: $CHANGELOG_EXIT)" + echo "❌ Could not generate changelog (exit code: $CHANGELOG_EXIT)" + EXECUTION_FAILED=true fi # Run breaking changes check @@ -201,26 +163,26 @@ echo "Processed: $PROCESSED_FILES/$TOTAL_FILES files" echo "========================================" # Summary +if [ "$EXECUTION_FAILED" = true ]; then + echo "❌ API diff execution failed" + exit 1 +fi + if [ "$BREAKING_CHANGES_FOUND" = true ]; then echo "" echo "❌ Breaking changes detected in the following files:" for file in "${FILES_WITH_BREAKING_CHANGES[@]}"; do echo " - $file" # Output GitHub Actions annotation - if [ -n "$GITHUB_ACTIONS" ]; then + if [ -n "${GITHUB_ACTIONS:-}" ]; then echo "::warning file=${file}::Breaking changes detected in this API spec file" fi done - if [ "$FAIL_ON_BREAKING" = true ]; then - echo "" - echo "Exiting with error due to breaking changes" - exit 1 - else - echo "" - echo "Note: Not failing build (use --fail-on-breaking to fail on breaking changes)" - fi + echo "" + echo "Exiting with error due to breaking changes" + exit 1 else echo "" echo "✓ No breaking changes detected across all files" -fi \ No newline at end of file +fi diff --git a/scripts/api-diff/api-diff.test.sh b/scripts/api-diff/api-diff.test.sh index bce73e8a9..d02963e32 100755 --- a/scripts/api-diff/api-diff.test.sh +++ b/scripts/api-diff/api-diff.test.sh @@ -1,91 +1,50 @@ -#!/bin/bash +#!/usr/bin/env bash -# Unit test for api-diff.sh conventional commit logic -# Tests commit message detection and FAIL_ON_BREAKING setting +set -euo pipefail -set -e +cd "$(dirname "$0")/../.." -echo "=== Unit Test: api-diff.sh Conventional Commit Logic ===" -echo +FAKE_BIN=$(mktemp -d) +trap 'rm -rf "$FAKE_BIN"' EXIT -TESTS_PASSED=0 -TESTS_FAILED=0 - -SCRIPT_PATH="scripts/api-diff/api-diff.sh" - -# Helper function to test script with commit messages -test_commit_messages() { - local commit_messages="$1" - local expected_mode="$2" # "allow-breaking-changes" or "block-breaking-changes" - local test_name="$3" - - echo "Testing: $test_name (commit: $commit_messages)" - - # Run the script in dry-run mode with COMMIT_MESSAGES set - local output - output=$(COMMIT_MESSAGES="$commit_messages" "$SCRIPT_PATH" --dry-run 2>&1) - - if [[ "$expected_mode" == "allow-breaking-changes" ]]; then - if echo "$output" | grep -q "Mode: Allowing breaking changes"; then - echo " ✓ PASS: Correctly allows breaking changes" - TESTS_PASSED=$((TESTS_PASSED + 1)) - else - echo " ✗ FAIL: Expected to allow breaking changes, but output was:" - echo "$output" - TESTS_FAILED=$((TESTS_FAILED + 1)) - fi - elif [[ "$expected_mode" == "block-breaking-changes" ]]; then - if echo "$output" | grep -q "Mode: Failing on breaking changes"; then - echo " ✓ PASS: Correctly fails on breaking changes" - TESTS_PASSED=$((TESTS_PASSED + 1)) - else - echo " ✗ FAIL: Expected to fail on breaking changes, but output was:" - echo "$output" - TESTS_FAILED=$((TESTS_FAILED + 1)) - fi +cat > "$FAKE_BIN/docker" <<'EOF' +#!/usr/bin/env bash +if [[ " $* " == *" changelog "* ]] && [ "${FAKE_DOCKER_MODE:-pass}" = "changelog-error" ]; then + echo "simulated changelog error" >&2 + exit 2 +fi +if [[ " $* " == *" breaking "* ]] && [ "${FAKE_DOCKER_MODE:-pass}" = "breaking" ]; then + echo "simulated breaking change" >&2 + exit 1 +fi +exit 0 +EOF +chmod +x "$FAKE_BIN/docker" + +run_check() { + local expected_exit=$1 + local mode=$2 + shift 2 + + set +e + env -u GITHUB_ACTIONS \ + PATH="$FAKE_BIN:$PATH" \ + BASE_BRANCH=origin/master \ + FAKE_DOCKER_MODE="$mode" \ + OASDIFF_DOCKER_IMAGE=test-image \ + bash scripts/api-diff/api-diff.sh "$@" >/dev/null 2>&1 + local actual_exit=$? + set -e + + if [ "$actual_exit" -ne "$expected_exit" ]; then + echo "Expected exit $expected_exit for mode '$mode', got $actual_exit" >&2 + exit 1 fi - echo } -# Test cases: test_commit_messages "commit_messages" "expected_mode" "test_name" -# expected_mode: "allow-breaking-changes" = allows breaking changes, "block-breaking-changes" = fails on breaking +run_check 0 pass xero_accounting.yaml +run_check 1 breaking xero_accounting.yaml +run_check 1 changelog-error xero_accounting.yaml +run_check 2 pass --unsupported -echo "--- Commit messages that SHOULD allow breaking changes ---" -test_commit_messages "feat!: remove deprecated endpoint" "allow-breaking-changes" "Header with ! marker" -test_commit_messages "feat(api)!: remove deprecated endpoint" "allow-breaking-changes" "Header with scope and ! marker" -test_commit_messages "feat: refactor API\n\nBREAKING CHANGE: response schema updated" "allow-breaking-changes" "BREAKING CHANGE footer" -test_commit_messages "fix: patch bug\n\nSome details\nBREAKING CHANGE: removed old field" "allow-breaking-changes" "BREAKING CHANGE footer after body" - -echo "--- Commit messages that SHOULD fail on breaking changes ---" -test_commit_messages "feat: add optional field" "block-breaking-changes" "Normal feature commit" -test_commit_messages "fix: resolve null issue" "block-breaking-changes" "Normal fix commit" -test_commit_messages "chore: update docs" "block-breaking-changes" "Chore commit" -test_commit_messages "docs: mention breaking behaviour in description" "block-breaking-changes" "Contains word breaking but no marker" - -echo "--- Test override with --fail-on-breaking ---" -# Test that --fail-on-breaking overrides commit message logic -echo "Testing override: commit with breaking marker and --fail-on-breaking" -output=$(COMMIT_MESSAGES="feat!: breaking api update" "$SCRIPT_PATH" --dry-run --fail-on-breaking 2>&1) -if echo "$output" | grep -q "Mode: Failing on breaking changes"; then - echo " ✓ PASS: --fail-on-breaking overrides commit logic" - TESTS_PASSED=$((TESTS_PASSED + 1)) -else - echo " ✗ FAIL: --fail-on-breaking did not override, output:" - echo "$output" - TESTS_FAILED=$((TESTS_FAILED + 1)) -fi -echo - -echo "========================================" -echo "Test Results:" -echo " Passed: $TESTS_PASSED" -echo " Failed: $TESTS_FAILED" -echo "========================================" - -if [ $TESTS_FAILED -gt 0 ]; then - echo "❌ Some tests failed!" - exit 1 -else - echo "✅ All tests passed!" - exit 0 -fi +echo "API diff fail-closed tests passed"