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.

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

Merged
merged 1 commit into from Sep 7, 2017

Conversation

Profpatsch
Copy link
Member

I hope this works.

@Profpatsch Profpatsch merged commit 6afa844 into NixOS:master Sep 7, 2017
@Ericson2314 Ericson2314 added this to the 17.09 milestone Sep 7, 2017
@Ericson2314 Ericson2314 added the 9.needs: port to stable A PR needs a backport to the stable release. label Sep 7, 2017
lib/ @Profpatsch

# Build-Support
pkgs/build-support @Profpatsch
Copy link
Member

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

Copy link
Member Author

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
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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"

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

@edolstra
Copy link
Member

edolstra commented Sep 7, 2017

I dislike that certain people are now "owner" of various parts of Nixpkgs, without any sort of decision procedure.

@Profpatsch
Copy link
Member Author

@edolstra I see it as a notification mechanism. Maybe we should add a comment at the top?

@samueldr samueldr removed the 9.needs: port to stable A PR needs a backport to the stable release. label Apr 17, 2019
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.

None yet

6 participants