Skip to content

Conversation

@wolfgangwalther
Copy link
Contributor

This enables the "Required Status Checks" feature for Nixpkgs' development branches.

At this stage, all nixpkgs committers can bypass the checks. See discussion in #130.

Notes:

cc @MattSturgeon

This enables the "Required Status Checks" feature for Nixpkgs'
development branches.

At this stage, all nixpkgs committers can bypass the checks.

Related: NixOS#130
@wolfgangwalther wolfgangwalther requested a review from a team as a code owner June 25, 2025 15:04
Copy link

@MattSturgeon MattSturgeon left a comment

Choose a reason for hiding this comment

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

  • Diff LGTM
  • Committers team id verified via rest api

This directory contains JSON exports of branch protection rulesets for repositories in the NixOS org.

They are not managed automatically, but can easily be imported by org owners.
To propose changes to branch protection rules, you can open a Pull Request.

For a second, I thought there was some magic github org-repo that'd could declaratively manage repo settings...

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

While I can't actually import this ruleset directly (gives an error), I can manually create it and then re-export it. Can I get another approval from an @NixOS/org owner for this? :)

@wolfgangwalther Thanks a lot for all this work, very appreciated!

@wolfgangwalther
Copy link
Contributor Author

While I can't actually import this ruleset directly (gives an error)

Interesting, what's wrong with it, I wonder? I guess we will see after export ;)

After applying parent commit
@infinisil infinisil force-pushed the rulesets-nixpkgs-required-status-checks branch from 3ef1003 to 78a5863 Compare July 10, 2025 20:50
@infinisil
Copy link
Member

Applied!

@infinisil
Copy link
Member

Btw the problem with the importing was just that the JSON list had an extra , at the end 😆

@infinisil infinisil merged commit 3cf61f2 into NixOS:main Jul 10, 2025
2 checks passed
@mweinelt
Copy link
Member

What prevents us from enabling merge queues now?

@infinisil
Copy link
Member

infinisil commented Jul 10, 2025

We could probably try that, I have the option to enable this for the ruleset:

Screenshot 2025-07-10 at 23-00-33 Settings · Rulesets · nixpkgs

@infinisil
Copy link
Member

infinisil commented Jul 10, 2025

These are also options, repo-wide:

image

@mweinelt
Copy link
Member

mweinelt commented Jul 10, 2025

Yes, auto-merge would make reviewing and merging async. That would be super helpful. And you can cancel until it is merged.

Actually, I care more for auto-merge than merge-queues. Merge queues would be an experiment, auto-merges should be fairly safe.

@wolfgangwalther wolfgangwalther deleted the rulesets-nixpkgs-required-status-checks branch July 11, 2025 06:44
@wolfgangwalther
Copy link
Contributor Author

re merge queues: before we can enable these, we need to split the ruleset etc. - we can't require them for the staging branches right now, because all our periodic merges need to bypass that etc.

re auto-merge: that's surely something we can try. we just need to consider that it won't wait for ofborg - it will only wait for eval and the other basic checks.

@MattSturgeon
Copy link

MattSturgeon commented Jul 11, 2025

These are also options, repo-wide:

image

FYI "Auto Merge" can be enabled either per-repo or per-ruleset. Also, I believe requiring "Merge Queues" implicitly enables "Auto Merge" for that ruleset, but I could be wrong.

What prevents us from enabling merge queues now?

We also need to add the on: merge_group event to the PR workflow; or add a separate workflow that runs on: merge_group but also has a no PR failures job.

Although I'm also looking forward to Merge Queues, I'd personally like to see the Required Status Checks working well in production before we rush into requiring a queue.

EDIT: We'd also need to consider how the ruleset targets branches, because Merge Queues cannot be used on rulesets that target wildcard branch patterns. E.g. we could have a "base" ruleset that uses wildcards, alongside a separate "overlay" ruleset that requires merge queues on explicitly named branches.

@mweinelt
Copy link
Member

This regressed our periodic merge CI.

https://github.com/NixOS/nixpkgs/actions/runs/16222366363/job/45805853015

@emilazy
Copy link
Member

emilazy commented Jul 11, 2025

staging* branches should probably be omitted from these kinds of changes by default since the workflow is significantly different. (It would be nice to make the workflow less different, but the reality is that direct pushes to staging-next etc. are still common.)

@infinisil
Copy link
Member

Staging is now not included anymore: #139

@wolfgangwalther
Copy link
Contributor Author

As mentioned in #139, I think we should keep this on staging, but allow the nixpkgs-ci app to bypass this ruleset as well. We actually discussed this issue already in #130 (comment) - but I entirely missed that we need nixpkgs-ci to be a bypasser as well.

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.

6 participants