Skip to content

Combine PR check workflows into one job#161

Open
danielparks wants to merge 1 commit into
mainfrom
combine-workflows
Open

Combine PR check workflows into one job#161
danielparks wants to merge 1 commit into
mainfrom
combine-workflows

Conversation

@danielparks
Copy link
Copy Markdown
Owner

Replaces the three separate pr-checks-*.yaml workflows (10 jobs total)
with a single job whose steps all run regardless of individual failures.
A summary step writes a pass/fail/duration table to GITHUB_STEP_SUMMARY
and fails the job if any check failed.

Co-Authored-By: Claude Sonnet 4.6 noreply@anthropic.com

Replaces the three separate pr-checks-*.yaml workflows (10 jobs total)
with a single job whose steps all run regardless of individual failures.
A summary step writes a pass/fail/duration table to GITHUB_STEP_SUMMARY
and fails the job if any check failed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment on lines +31 to +38
- uses: actions-rust-lang/setup-rust-toolchain@46268bd060767258de96ed93c1251119784f2ab6 # v1.16.1
with:
components: clippy

- uses: actions-rust-lang/setup-rust-toolchain@46268bd060767258de96ed93c1251119784f2ab6 # v1.16.1
with:
toolchain: nightly
components: rustfmt
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Calling setup-rust-toolchain twice is worth double-checking. The action may set RUSTUP_TOOLCHAIN (or equivalent) to the last toolchain it installs, which would make nightly the effective default for any cargo invocation that doesn't use +toolchain. That would mean cargo clippy, cargo doc, cargo build, and cargo test silently run on nightly instead of stable — different from the original separate-job setup.

You can avoid the ambiguity by combining into one call and letting +nightly in the cargo fmt step handle the toolchain switch explicitly:

Suggested change
- uses: actions-rust-lang/setup-rust-toolchain@46268bd060767258de96ed93c1251119784f2ab6 # v1.16.1
with:
components: clippy
- uses: actions-rust-lang/setup-rust-toolchain@46268bd060767258de96ed93c1251119784f2ab6 # v1.16.1
with:
toolchain: nightly
components: rustfmt
- uses: actions-rust-lang/setup-rust-toolchain@46268bd060767258de96ed93c1251119784f2ab6 # v1.16.1
with:
components: clippy
toolchain: stable
- uses: actions-rust-lang/setup-rust-toolchain@46268bd060767258de96ed93c1251119784f2ab6 # v1.16.1
with:
toolchain: nightly
components: rustfmt

Or look at whether the action supports listing multiple toolchains in a single call. Either way, verifying that cargo clippy is actually running on stable in CI would be a good smoke-check.

Comment on lines +57 to +64
- name: zizmor
id: zizmor
continue-on-error: true
run: |
start=$SECONDS
zizmor . || rc=$?
echo "duration=$((SECONDS - start))" >> "$GITHUB_OUTPUT"
exit ${rc:-0}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The original pr-checks-other.yaml used the zizmorcore/zizmor-action with permissions: security-events: write, which uploads SARIF results to the GitHub Security tab. Switching to the CLI drops that integration — zizmor findings will only appear in the step log, not in the Security → Code scanning view.

If SARIF reporting matters, you'd need to add something like:

      - name: zizmor
        id: zizmor
        continue-on-error: true
        run: |
          start=$SECONDS
          zizmor --format sarif . > zizmor.sarif || rc=$?
          echo "duration=$((SECONDS - start))" >> "$GITHUB_OUTPUT"
          exit ${rc:-0}

      - name: Upload zizmor SARIF
        if: always()
        uses: github/codeql-action/upload-sarif@...
        with:
          sarif_file: zizmor.sarif

…and add security-events: write to the job permissions. Whether that integration was actually being used and is worth the added complexity is a judgment call — just flagging the regression.

@claude
Copy link
Copy Markdown

claude Bot commented May 30, 2026

Clean consolidation overall. The continue-on-error + summary pattern works well and the step-timing output is a nice touch.

Two issues flagged inline:

  1. Double setup-rust-toolchain (lines 31–38) — calling the action twice may leave nightly as the default toolchain, so cargo clippy, cargo doc, cargo build, and cargo test could silently run on nightly instead of stable. Worth verifying in a test run.

  2. zizmor SARIF upload is gone — the original zizmorcore/zizmor-action uploaded findings to GitHub's Security tab via SARIF. The CLI replacement drops that. Whether it's worth restoring is a judgment call.

One minor thing not worth an inline: cargo build --tests --all-features before cargo test --all-features is carried over from the original, but with continue-on-error: true on both, a build failure will still let cargo test run (and fail again). It does serve as useful signal isolation in the summary table, so it's defensible.

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.

2 participants