Split cargo-deny job into two non-matrix jobs #1670
Conversation
Instead of conditionally applying `continue-on-error: true` at the job level to the `advisories` job, this splits `cargo-deny` into two job definitions, `cargo-deny-advisories` and `cargo-deny`, where *neither* has `continue-on-error` but `cargo-deny-advisories` is omitted as a dependency of the `tests-pass` job that makes jobs effectively required for PR auto-merge. This way, when there is an unaddressed advisory, the `cargo-deny-advisories` job unambiguously fails, even failing the workflow, but PRs can still auto-merge. One implication of this is that, on Dependabot security update PRs, `@dependabot merge` and `@dependabot squash and merge` commands will only perform a merge if `cargo deny check advisories` reports no other outstanding advisories. This is because, when Dependabot is told to merge a PR, it only goes ahead with the merge if all checks pass (i.e. report a successful conclusion). This would be convenient for cases where, if the fix is not complete, further manual review is desired. It would otherwise be inconvenient, but then a usual PR auto-merge could be done instead (which is the more common practice here anyway).
EliahKagan
force-pushed
the
run-ci/cargo-deny
branch
from
38edb2c to
a52a2a1
Compare
|
I've rebased this and it should be ready to go (once tests pass). |
|
For future reference, in case it ever changes: That Dependabot commands only merge when all checks pass, and not on the weaker condition of all required checks for auto-merge passing, seems to me to follow directly from the phrase "once your CI tests have passed" in the documentation, as well as to be the most intuitive behavior. After all, when people prefer the behavior of auto-merge (as we often would here), they can just use that. But this is subjective and some people consider it a bug that Dependabot does not ignore non-required checks. See dependabot/dependabot-core#7383. So this behavior may change some day. In any case, getting this Dependabot behavior was not the primary motivation for the changes in this PR. |
This is the WASM de-matrixification that was intentionally omitted from #1668 so it could be considered separately (see also #1668 (review)).
This PR is intended to have only one commit, 38edb2c, and the changes are really +12 -9, not +1201 -742 as currently shown. The other commits are from #1668. This is currently a draft because I recommend against merging it until #1668 is merged, and it may also be a good idea to rebase this after #1668 is merged, which will make it easy to verify that merging this will only add one commit.
What this changes
Instead of conditionally applying
continue-on-error: trueat the job level to theadvisoriesjob, this splitscargo-denyinto two job definitions,cargo-deny-advisoriesandcargo-deny, where neither hascontinue-on-errorbutcargo-deny-advisoriesis omitted as a dependency of thetests-passjob that makes jobs effectively required for PR auto-merge. This way, when there is an unaddressed advisory, thecargo-deny-advisoriesjob unambiguously fails, even failing the workflow, but PRs can still auto-merge.One implication of this is that, on Dependabot security update PRs,
@dependabot mergeand@dependabot squash and mergecommands will only perform a merge ifcargo deny check advisoriesreports no other outstanding advisories. This is because, when Dependabot is told to merge a PR, it only goes ahead with the merge if all checks pass (i.e. report a successful conclusion). This would be convenient for cases where, if the fix is not complete, further manual review is desired. It would otherwise be inconvenient, but then a usual PR auto-merge could be done instead (which is the more common practice here anyway).Possible alternative
The main thing I can think of that would make this change unsuitable is if we actually just want to start treating
cargo deny check advisoriesfailures as hard errors and have them block merging. In that case, the current organization could be kept and the conditionalcontinue-on-errorcould just be removed from it.