ci: fix pre-commit job still passing when pre-commit fails#5790
ci: fix pre-commit job still passing when pre-commit fails#5790jyotshnayaparla-00 merged 1 commit intomainfrom
Conversation
873e88a to
3385feb
Compare
gz
left a comment
There was a problem hiding this comment.
it makes sense, did you test if it fixes the regression?
I didn't test it directly. I can verify by checking if the pre-commit failures from the other PR (#5786 ) now correctly fail the job instead of passing. Once #5786 's pre-commit check runs with this fix in place, it should correctly fail the job instead of passing. We'll know right away from that run. Alternatively, happy to test it in feldera-ci-test first if you'd prefer. |
mythical-fred
left a comment
There was a problem hiding this comment.
LGTM. Removing continue-on-error plugs the hole where pre-commit failures were silently passing on branch PRs, and always() on the auto-fix step keeps the auto-commit running even after a failure — exactly right.
Remove continue-on-error from the pre-commit step and move the same-repo condition to the auto-commit step using always(). This ensures the job is correctly marked as failed when pre-commit fails, while still allowing the auto-fix commit to run on branches within this repo.
3385feb to
ccee0ac
Compare
Problem: continue-on-error was set conditionally, true for branches in this repo, false for forks. The intent was to allow the auto-fix commit step to run even when pre-commit fails. However, continue-on-error: true at the step level also causes the entire job to be marked as succeeded, meaning pre-commit failures on branches within this repo were silently ignored.
For fork PRs, continue-on-error was false, so pre-commit failures correctly failed the job, but the auto-fix commit step would never run (which is fine since we can't push to forks anyway).
Fix: Remove continue-on-error from the pre-commit step entirely, so pre-commit failures always fail the job regardless of whether the PR is from a fork or a branch in this repo. Move the same-repo condition to the auto-commit step using if: always() && head_repo == this_repo, so the auto-fix commit still runs after a pre-commit failure on branches within this repo, while correctly skipping it for forks.