Skip to content

Move MSRV jobs into main ci.yml workflow #2027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 26, 2025

Conversation

EliahKagan
Copy link
Member

@EliahKagan EliahKagan commented May 25, 2025

This moves the check-msrv and check-msrv-badge job definitions from msrv.yml to ci.yml, where they are renamed msrv and msrv-badge, respectively.

This includes those jobs among the PR-blocking checks in ci.yml (i.e., tests-pass depends on them). This lets failure of msrv continue blocking PR auto-merge (check-msrv had done so), and causes msrv-badge to begin blocking PRs (check-msrv-badge had not done so).

The msrv.yml workflow is accordingly removed. See #2026 for relevant context and discussion. This should only be merged if I am correct to think that these no longer benefit from being separate workflows.

If this is merged, then branch protection rules should be adjusted to remove the required MSRV checks now that they are covered by tests-pass in ci.yml. However, it looks like nothing will break in between merging this and removing those required checks: moving a job from one workflow to another, and even renaming it in the sense of changing its job ID (the YAML key that introduces it), does not break a required check for the job, so long as it had a name key that was unchanged!

One integration subtlety here is that, if those required checks are removed, then preexisting PRs could potentially auto-merge even if they fail MSRV checks. This could be addressed by manually verifying that they pass the checks; or by rebasing them onto main; or by waiting to remove the old required MSRV checks until any preexisting PRs are rebased onto main, merged, or closed; or by not worrying about it if no MSRV-related changes are expected in any of them.


This rests right on the line between what I would readily merge without a review and what I would wait for a review on. The only disadvantage I can think of here is if running MSRV checks, and no others, through workflow-dispatch, is desired. I don't think that's an important thing to be able to do, but if it is then the msrv.yml workflow should be kept and this PR should not be merged.

The change here is fairly non-disruptive and could be reverted... but the resulting commit history would be somewhat harder to understand. Since this doesn't seem to be blocking anything--in particular, I don't have any further changes to make to the details of the MSRV jobs at this time--I think I'll wait for a review.

This moves the `check-msrv` and `check-msrv-badge` job definitions
from `msrv.yml` to `ci.yml`, where they are renamed `msrv` and
`msrv-badge`, respectively.

This includes those jobs among the PR-blocking checks in `ci.yml`
(i.e., `tests-pass` depends on them). This lets failure of `msrv`
continue blocking PR auto-merge (`check-msrv` had done so), and
causes `msrv-badge` to begin blocking PRs (`check-msrv-badge` had
not done so).

The `msrv.yml` workflow is accordingly removed.

(See GitoxideLabs#2026 and GitoxideLabs#2027 for context and relevant discussion.)
@EliahKagan EliahKagan marked this pull request as ready for review May 25, 2025 22:03
@EliahKagan EliahKagan requested a review from Byron May 25, 2025 22:03
@Byron
Copy link
Member

Byron commented May 26, 2025

Alright, let's do it!

I wonder if PRs that don't have a particular name key would pass in their absence. But I suppose we will find out as the required jobs list now looks like this:

Screenshot 2025-05-26 at 05 08 13

I will adjust these as needed after a while or if something does fail.

@Byron Byron enabled auto-merge May 26, 2025 03:10
@EliahKagan
Copy link
Member Author

EliahKagan commented May 26, 2025

The prediction in the description here did not account for the effect of the "MSRV checks pass" job being added as required. Unlike the others, that job is not preserved in the changes in this PR -- since the preexisting "Tests pass" job now covers the MSRV checks.

As a result, auto-merge cannot proceed here while the "MSRV checks pass" job remains set to required in the branch protection rules. (Although it would be possible to override the rules and merge this, that should definitely not be done, since it would cause the same problem to befall future PRs. Instead, if this PR is to be merged, then the "MSRV checks pass" job should be removed as a required check in the branch protection rules.)

@Byron
Copy link
Member

Byron commented May 26, 2025

Right, that's an instant answer for my question about required checks as well :).

@Byron Byron merged commit 5950c58 into GitoxideLabs:main May 26, 2025
23 checks passed
@EliahKagan EliahKagan deleted the run-ci/combine-workflows branch May 26, 2025 03:51
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