Skip to content

test: md-only PR to verify ci-complete gate#1285

Closed
spomichter wants to merge 2 commits intodevfrom
test/md-only-ci-gate
Closed

test: md-only PR to verify ci-complete gate#1285
spomichter wants to merge 2 commits intodevfrom
test/md-only-ci-gate

Conversation

@spomichter
Copy link
Contributor

Pure md-only change off dev (after #1284 merged). Verifies:

  1. Workflow triggers (no paths-ignore blocking)
  2. check-changes detects no code changes (all outputs false)
  3. should-run evaluates to false for all test callers
  4. Test jobs complete instantly (steps skipped)
  5. ci-complete passes

Do NOT merge — delete after verification.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Feb 17, 2026

Greptile Summary

This PR bundles two changes: (1) a temporary markdown file (docs/ci-gate-test.md) originally intended to test md-only CI behavior, and (2) a substantive CI fix to docker.yml that moves path-filter checks from within the tests.yml should-run input to job-level if: conditions, and expands ci-complete's needs to include build jobs.

Changes and impact:

  • Test job if conditions (run-ros-tests, run-tests, run-heavy-tests, run-lcm-tests, run-integration-tests, run-mypy): All six jobs now have explicit if: guards that short-circuit entirely (result = skipped) when no relevant files changed. Previously they always ran but with should-run: false (spinning up the job runner with no actual work). The always() prefix correctly allows evaluation even when upstream build jobs (dev, ros-dev) are skipped, so the conditions can still check check-changes outputs.
  • ci-complete expanded needs: The gate now also waits on ros, python, ros-python, dev, and ros-dev build jobs, closing a gap where a build failure would not have been caught.
  • Self-verification caveat: docker.yml is itself listed in the ros and python path filters. Because this PR modifies docker.yml, check-changes will output ros=true and python=true for this PR's CI run, meaning the md-only skip path described in the PR description will not be exercised by this PR's own run.

Issues found:

  • check-changes is not in ci-complete's needs, so a check-changes failure silently passes the gate (pre-existing, but addressable now).
  • docs/ci-gate-test.md instructs itself to be deleted after verification but is included in the PR; the verification it was created for cannot be demonstrated by this PR due to the docker.yml change also being present.

Confidence Score: 4/5

  • The workflow logic is correct and the CI gate improvement is sound; minor gaps remain around check-changes failure propagation and the temporary test file.
  • The core workflow changes are logically correct: job-level if conditions properly skip test jobs for non-code PRs, always() is used appropriately so skipped upstream jobs don't prevent condition evaluation, and expanding ci-complete's needs to include build jobs closes a real gap. No functional regressions are introduced. Score is 4 rather than 5 because: (1) check-changes is still not in ci-complete's direct needs (silently swallows infrastructure failures), and (2) the temporary docs/ci-gate-test.md file has a "delete after verification" note but appears targeted for merge as-is.
  • .github/workflows/docker.yml line 309 (ci-complete needs list) and docs/ci-gate-test.md (temporary file intended for deletion).

Important Files Changed

Filename Overview
.github/workflows/docker.yml Adds job-level if conditions to all test caller jobs so they are fully skipped for non-code PRs; also expands ci-complete's needs to include build jobs (ros, python, ros-python, dev, ros-dev) so build failures are caught by the gate. Logic is sound for md-only PRs but this PR itself triggers the ros/python path filters (docker.yml is tracked), so the md-only skip behavior cannot be self-verified by this PR's own run.
docs/ci-gate-test.md Temporary 4-line markdown file intended to trigger a md-only CI run for verification; marked for deletion after verification. No issues.

Flowchart

flowchart TD
    PR[Pull Request / Push] --> CC[check-changes\ndorny/paths-filter]

    CC -->|ros outputs| ROS{ros == true?}
    CC -->|python outputs| PY{python == true?}
    CC -->|dev outputs| DEV_OUT{dev == true?}
    CC -->|tests outputs| TST{tests == true?}

    ROS -->|yes| ROS_JOB[ros build]
    ROS -->|no| ROS_SKIP[skipped]
    PY -->|yes| PY_JOB[python build]
    PY -->|no| PY_SKIP[skipped]

    ROS_JOB --> ROS_PY[ros-python build\nif: always]
    ROS_SKIP --> ROS_PY
    PY_JOB --> DEV_JOB[dev build\nif: always]
    PY_SKIP --> DEV_JOB
    ROS_PY --> ROS_DEV[ros-dev build\nif: always]

    CC -->|"NEW: if: always() &&\ncheck-changes==success &&\n(tests OR ros OR python OR dev)"| RT[run-ros-tests]
    CC -->|"NEW: if: always() &&\ncheck-changes==success &&\n(tests OR python OR dev)"| RTT[run-tests\nrun-heavy-tests\nrun-lcm-tests\nrun-integration-tests]
    CC -->|"NEW: same as ros-tests"| RM[run-mypy]
    ROS_DEV --> RT
    ROS_DEV --> RM
    DEV_JOB --> RTT

    ROS_JOB --> CI_COMPLETE
    PY_JOB --> CI_COMPLETE
    ROS_PY --> CI_COMPLETE
    DEV_JOB --> CI_COMPLETE
    ROS_DEV --> CI_COMPLETE
    RT --> CI_COMPLETE
    RTT --> CI_COMPLETE
    RM --> CI_COMPLETE

    CI_COMPLETE{ci-complete\nif: always\nNEW: also needs build jobs}
    CI_COMPLETE -->|contains failure/cancelled| FAIL[❌ CI Failed]
    CI_COMPLETE -->|only success/skipped| PASS[✅ CI Passed]

    style FAIL fill:#f55,color:#fff
    style PASS fill:#5a5,color:#fff
    style CI_COMPLETE fill:#fa0,color:#000
Loading

Last reviewed commit: 2837432

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@spomichter spomichter closed this Feb 17, 2026
…omplete

Move path filter checks to job-level if: conditions so test caller
jobs are fully skipped (no container spin-up) when no relevant files
changed. The always() is kept so skipped upstream builds don't block
the condition from evaluating.

Also add build jobs (ros, python, ros-python, dev, ros-dev) to
ci-complete's needs list so build failures are caught by the gate.
@spomichter spomichter reopened this Feb 17, 2026
@spomichter
Copy link
Contributor Author

Closing — contains workflow changes in diff, not a clean md-only test.

@spomichter spomichter closed this Feb 17, 2026
@spomichter spomichter deleted the test/md-only-ci-gate branch February 17, 2026 19:10
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@@ -271,7 +309,7 @@ jobs:
# /entrypoint.sh bash -c "pytest -m module"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ci-complete does not catch check-changes failures

ci-complete's needs list does not include check-changes directly. If check-changes fails, all downstream jobs that don't use if: always() (e.g. ros, python) are automatically skipped by GitHub Actions — they won't appear as failure in needs.*.result. As a result, a check-changes failure would cause ci-complete to pass (no failure or cancelled in its direct dependencies), silently swallowing the infrastructure failure.

Consider adding check-changes to ci-complete's needs list:

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]

This is a pre-existing gap, but the expansion of ci-complete's needs in this PR is a good opportunity to close it.

Comment on lines +1 to +4
# CI Gate Verification

Test file to verify md-only PRs skip the test suite and pass ci-complete.
Delete after verification.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary file not deleted after verification

The file itself says "Delete after verification." Since this PR also changes docker.yml (which is tracked by the ros and python path filters), the CI run for this PR will trigger full builds and tests — it won't self-verify the md-only skip behavior described in the PR description. A follow-up PR with only an .md change would be needed to observe the skipping logic in action.

If this file is being kept as a persistent example or documentation, the "Delete after verification" note should be removed. Otherwise, this file should be removed before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant