fix(ci): skip test jobs entirely for non-code PRs, add builds to ci-complete

[spomichter]

Changes

1. Job-level gating on test callers — moves path filter checks from should-run input to if: condition at the job level. For md-only PRs, test jobs are fully skipped (no container spin-up, no runner allocation).

2. Build jobs added to ci-complete — ros, python, ros-python, dev, ros-dev are now in ci-complete's needs list. Build failures are caught by the gate.

always() is kept in the if: so that skipped upstream builds don't prevent the condition from evaluating.

After merging

Open a pure md-only PR against dev to verify test jobs show as skipped and ci-complete passes.

[greptile-apps]

Greptile Summary

This PR makes two targeted CI improvements to docker.yml:

1. Job-level test gating: Adds an if: condition to all six test caller jobs (run-ros-tests, run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests, run-mypy) so they are entirely skipped — no runner allocation, no container pull — when none of the relevant path-filter outputs are true. Previously, if: always() caused the job to run but all steps were individually skipped via the should-run input, still paying the container spin-up cost.

2. Build jobs added to ci-complete: ros, python, ros-python, dev, and ros-dev are now in ci-complete's needs list so that build failures block merges through the existing failure/cancelled gate.

• The always() is correctly retained inside the new compound if: expressions, ensuring test jobs aren't silently skipped when an upstream build job fails.
• The should-run input passed to tests.yml is now always true when a test job runs (the job-level if: is the same expression), making the per-step guards in tests.yml redundant dead code — minor cleanup opportunity.
• check-changes is still absent from ci-complete's needs list: if check-changes fails, all downstream jobs end up skipped/successful and ci-complete passes, creating a potential false-green on runner or checkout failures.

Confidence Score: 4/5

• Safe to merge — the logic changes are correct and the efficiency gain is real; one pre-existing gap (check-changes not gating ci-complete) is noted but not introduced by this PR.
• The job-level if: gating is logically sound and the always() placement is correct. Build jobs being added to ci-complete is a straightforward improvement. The should-run redundancy is a minor style issue. The check-changes absence from ci-complete is a pre-existing gap worth noting but does not regress existing behavior — it was already present before this PR.
• .github/workflows/docker.yml lines 312 (ci-complete needs list) and 158–164 (redundant should-run).

Important Files Changed

Filename	Overview
.github/workflows/docker.yml	Adds job-level if: gating to all 6 test jobs to skip them entirely for non-code PRs, and adds 5 build jobs (ros, python, ros-python, dev, ros-dev) to ci-complete's needs. The logic is correct but the should-run input passed to test workflows is now always-true when a test job runs (redundant), and check-changes is still absent from ci-complete's needs.

Flowchart

flowchart TD
    A[check-changes\ndorny/paths-filter] -->|ros/python/dev/tests outputs| B{Path filter outputs}

    B -->|ros==true| C[ros\nbuild job]
    B -->|python==true| D[python\nbuild job]
    B -->|always| E[ros-python\nif always]
    B -->|always| F[dev\nif always]
    B -->|always| G[ros-dev\nif always]

    C --> E
    D --> F
    E --> G

    B -->|NEW: if always AND check-changes==success AND outputs| H{Test gate\nnew job-level if}
    G --> H
    F --> H

    H -->|ros/python/dev/tests==true| I[run-ros-tests]
    H -->|python/dev/tests==true| J[run-tests]
    H -->|python/dev/tests==true| K[run-heavy-tests]
    H -->|python/dev/tests==true| L[run-lcm-tests]
    H -->|python/dev/tests==true| M[run-integration-tests]
    H -->|ros/python/dev/tests==true| N[run-mypy]
    H -->|all flags false\nmd-only PR| O[SKIP - no runner\nno container spin-up]

    C --> P[ci-complete\nNEW: includes build jobs]
    D --> P
    E --> P
    F --> P
    G --> P
    I --> P
    J --> P
    K --> P
    L --> P
    M --> P
    N --> P

    P -->|contains failure/cancelled| Q[❌ CI Gate Fails\nexit 1]
    P -->|all success/skipped| R[✅ CI Passed]

Loading

_{Last reviewed commit: 70c1fb2}

[greptile-apps]

_{1 file reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

check-changes absent from ci-complete needs — failure can go undetected

check-changes is not in ci-complete's needs list. If check-changes fails:

• ros and python are skipped (their if: outputs evaluate to empty/false).
• ros-python, dev, and ros-dev run (have if: always()) but all their steps are skipped because should-run depends on check-changes.result == 'success', so they report success.
• All six test jobs are skipped because the new if: condition gates on check-changes.result == 'success'.
• ci-complete sees only skipped/success results and passes, giving a false green.

Adding check-changes to the needs list ensures a runner setup failure or permission error doesn't silently allow a merge:

Suggested change

	needs: [ros, python, ros-python, dev, ros-dev, run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests, run-ros-tests, run-mypy]
	needs: [check-changes, ros, python, ros-python, dev, ros-dev, run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests, run-ros-tests, run-mypy]

[spomichter]

true fixing

[greptile-apps]

Additional Comments (1)

.github/workflows/docker.yml
should-run is now always true when this job runs

The job-level if: condition (lines 147–154) and the should-run expression passed to tests.yml are now identical. Because the job only runs when the if: passes, should-run will always evaluate to true inside tests.yml, making the per-step if: ${{ inputs.should-run }} guards in tests.yml dead code for this call site.

This is harmless today, but it creates a misleading impression that should-run: false can still be reached, and it means the tests.yml internal guards are no longer providing real protection. You can simplify by passing should-run: true explicitly (or dropping it entirely since the default is true), making the intent clear:

    with:
      should-run: true
      cmd: "pytest && pytest -m ros" # run tests that depend on ros as well

Same applies to run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests, and run-mypy.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}