Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Mass pings be gone, code owners for all #347610

Merged
merged 4 commits into from
Oct 9, 2024
Merged

Mass pings be gone, code owners for all #347610

merged 4 commits into from
Oct 9, 2024
+407 −459

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Oct 9, 2024 •
edited
Loading

This PR effectively disables the native GitHub codeowners feature and enables the new alternate codeowners mechanism introduced in #336261, meaning that:

  • Targeting the wrong branch won't trigger mass pings anymore, and if the wrong base branch is chosen, a helpful comment is posted telling you how to fix it, so no need to worry about the wrong base branch anymore! (though it might take some time for this to be propagated to non-master branches)
  • We can now declare users without write access as code owners!

Things done

This work is sponsored by Antithesis ✨

Add a 👍 reaction to pull requests you find important.

@infinisil infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Oct 9, 2024
@infinisil infinisil mentioned this pull request Oct 9, 2024
Merged
9 tasks
emilazy
emilazy reviewed Oct 9, 2024
View reviewed changes
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

A shame that we don’t get syntax highlighting any more, I wonder if we could name it CODEOWNERS outside of .github or something to benefit from the nice colouring (and to avoid stepping on GitHub’s namespace)?

.github/OWNERS Outdated
/nixos/modules/virtualisation/qemu-vm.nix @raitobezarius

# ACME
/nixos/modules/security/acme @arianvp @flokli @aanderse @emilazy # no merge permission: @m1cr0man
Copy link
Member

Choose a reason for hiding this comment

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

This could be @NixOS/acme now :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to leave this up to the individual people in follow-up PRs, so that we don't change too much in this one

Copy link
Member

Choose a reason for hiding this comment

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

Yeah for sure, was just happy about it.

RossComputerGuy
RossComputerGuy approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@RossComputerGuy RossComputerGuy left a comment

Choose a reason for hiding this comment

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

LGTM

@RossComputerGuy
Copy link
Member

A shame that we don’t get syntax highlighting any more, I wonder if we could name it CODEOWNERS outside of .github or something to benefit from the nice colouring

Maybe that vim set fmt comment thing might work?

@infinisil
Copy link
Member Author

infinisil commented Oct 9, 2024 •
edited
Loading

A shame that we don’t get syntax highlighting any more, I wonder if we could name it CODEOWNERS outside of .github or something to benefit from the nice colouring (and to avoid stepping on GitHub’s namespace)?

Oh damn, that actually works: https://github.com/Infinisil-s-Test-Organization/nixpkgs/blob/master/ci/CODEOWNERS (also, here's why), though it would also complain about users not having write access in the UI, meh :/

@emilazy
Copy link
Member

emilazy commented Oct 9, 2024

I think I’d prefer the warning to no syntax highlighting, unless there’s a way we can get both, but it’s not a big deal. Putting it under ci/ seems better than .github/ in any case.

@emilazy
Copy link
Member

emilazy commented Oct 9, 2024

Ah, we can do this: https://github.com/github-linguist/linguist/blob/f164d13fa618023ecf2d8f2ed9a6ce5fae731346/docs/overrides.md

@infinisil infinisil marked this pull request as draft October 9, 2024 23:06
@emilazy
Copy link
Member

emilazy commented Oct 9, 2024 •
edited
Loading

I think if we move it to ci/OWNERS and add this to .gitattributes then syntax highlighting should work:

ci/OWNERS linguist-language=CODEOWNERS

@infinisil
Copy link
Member Author

I'm not convinced that syntax highlighting at the cost of constant warnings is worth it tbh. Will move to ci/OWNERS though.

@infinisil
CODEOWNERS: Switch to alternate mechanism
b01ca00 
This effectively disables the native GitHub codeowners feature
and enables the new alternate codeowners mechanism introduced in
NixOS#336261

This means that:
- We can now declare users without write access as code owners!
- Targeting the wrong branch won't trigger mass pings anymore!
@emilazy
Copy link
Member

emilazy commented Oct 9, 2024

I suspect we might not get warnings if you set it in .gitattributes, but I can’t say for sure.

@infinisil
CONTRIBUTING: Remove now-unneeded mass ping section
c246403 
This is not a problem anymore with the parent commit
@infinisil
ci/request-reviews: Make wrong base branch message hashes more transp…
c721e91 
…arent

Also, fix the description of the text
@infinisil
Copy link
Member Author

I suspect we might not get warnings if you set it in .gitattributes, but I can’t say for sure.

Whoa, that actually does work, very nice! I also verified it in another repo because I couldn't believe it at first, but it really does work!

@infinisil infinisil marked this pull request as ready for review October 9, 2024 23:47
SigmaSquadron
SigmaSquadron approved these changes Oct 9, 2024
View reviewed changes
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment

Choose a reason for hiding this comment

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

LGTM; Thank you for implementing CODEOWNERS for the rest of us!

@infinisil infinisil changed the title Mass pings be gone Mass pings be gone, code owners for all Oct 9, 2024
@emilazy
Copy link
Member

emilazy commented Oct 9, 2024

Whoa, that actually does work, very nice! I also verified it in another repo because I couldn't believe it at first, but it really does work!

My intuition was that the component that handles the Linguist‐based syntax highlighting is distinct from the component that checks the syntax and handles displaying the warnings in the UI. Looks like I was right :)

emilazy
emilazy approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

Let’s try it out. Thanks for tackling this long‐standing problem!

@emilazy emilazy merged commit 27272c2 into NixOS:master Oct 9, 2024
17 of 18 checks passed
@infinisil infinisil deleted the codeowners-switch branch October 9, 2024 23:52
@infinisil infinisil mentioned this pull request Oct 10, 2024
Draft
@infinisil
Copy link
Member Author

It works! #347626

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emilazy emilazy emilazy approved these changes

@RossComputerGuy RossComputerGuy RossComputerGuy approved these changes

@SigmaSquadron SigmaSquadron SigmaSquadron approved these changes

@philiptaron philiptaron Awaiting requested review from philiptaron

@K900 K900 Awaiting requested review from K900

@winterqt winterqt Awaiting requested review from winterqt

@cafkafk cafkafk Awaiting requested review from cafkafk

Assignees
No one assigned
Labels
6.topic: policy discussion significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@infinisil @RossComputerGuy @emilazy @SigmaSquadron