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
CODEOWNERS: add Profpatsch to various paths #29061
Conversation
lib/ @Profpatsch | ||
|
||
# Build-Support | ||
pkgs/build-support @Profpatsch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last matching pattern counts, so this should go before my cc-wrapper one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, sorry. Will fix in a new Pr.
@@ -7,15 +7,26 @@ | |||
# For documentation on this file, see https://help.github.com/articles/about-codeowners/ | |||
# Mentioned users will get code review requests. | |||
|
|||
# Boostraping and core infra | |||
pkgs/stdenv/ @Ericson2314 | |||
pkgs/build-support/cc-wrapper/ @Ericson2314 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, who made @Ericson2314 owner of stdenv?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I believe our current use of CODEOWNERS
is mainly for people to express interest in being pinged (automatically) about changes in some files, even though the feature can be used in much stronger ways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the name of the file, and Github's description states it's clearly about ownership.
I didn't know that you could require review from code owners. That might be a good idea to enable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they consider it that way, but so far in nixpkgs there doesn't seem much interest in strict code ownership. We might explicitly document our intentions at the top of the file, though it's a little risky deviating too much from the semantics envisioned by github.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be a decent idea to use "code owners" to replace the mention bot (and I feel that had been mentioned somewhere). Rather than a strict definition of "owning" the code, it would be more in the sense of "I'd like to be invited to review PRs of this code"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah, I see it as replacement of mention bot as well.
I added myself to every file I want to be notified of when it changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All I wanted was notifications. I don't view it as my fief.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@edolstra I'd be worried that if we did use it as an enforcement mechanism, then there'd be no way for non-owners to selectively subscribe to PRs. I want to be able to stay abreast of stdenv changes without needing to also convince you or anyone I should also be a final arbiter of PR approval.
I dislike that certain people are now "owner" of various parts of Nixpkgs, without any sort of decision procedure. |
@edolstra I see it as a notification mechanism. Maybe we should add a comment at the top? |
I hope this works.