Skip to content

Create CODEOWNERS#2924

Merged
Mariatta merged 3 commits intomasterfrom
Mariatta-codeowners
Aug 1, 2017
Merged

Create CODEOWNERS#2924
Mariatta merged 3 commits intomasterfrom
Mariatta-codeowners

Conversation

@Mariatta
Copy link
Member

Copied over info from .mention-bot

python/core-workflow#160

Copied over info from .mention-bot
@Mariatta
Copy link
Member Author

🤔 wondering if lowercase codeowners file will work ...

Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Might as well delete .mention-bot while we're at it.

@@ -0,0 +1,11 @@
Doc/library/hashlib.rst @tiran
Copy link
Member

Choose a reason for hiding this comment

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

Could this be collapsed with Lib/hashlib.py with hashlib.*? Same for ssl.*? And if you really want to catch everything including tests with one rule then would *hashlib.* work?

Modules/_ssl* @tiran
Modules/hashlib.h @tiran
Python/pyhash.c @tiran
Include/pyhash.h @tiran
Copy link
Member

Choose a reason for hiding this comment

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

pyhash.* should cover both the .c and .h files.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I'm thinking to replace with the following patterns. What do you think? cc @tiran

# Hashing
**/*hashlib*                  @tiran
**/*pyhash*                   @tiran

# SSL
**/*ssl*                      @tiran

@@ -0,0 +1,11 @@
Doc/library/hashlib.rst @tiran
Copy link
Member

Choose a reason for hiding this comment

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

Since this file might grow a lot, we should probably have a comment per section to say what the rules are meant for, e.g. # Hashing.

@brettcannon
Copy link
Member

@tiran since he would be the inaugural user of the file. 😄

@Mariatta
Copy link
Member Author

mention-bot deletion is in #2923 :)

@brettcannon
Copy link
Member

Then if @tiran is happy with the results and we add a section comment so we know thematically what the rules are meant to be for, then it LGTM to merge this and delete the .mention-bot file!

@Mariatta Mariatta requested a review from tiran July 28, 2017 22:33
@tiran
Copy link
Member

tiran commented Jul 29, 2017

Could we auto-generate CODEOWNERS from .mentionbot? Or have a script that generates both from a simple rule file? Also mentionbot configuration and code ownership are not quite the same. For example I'm just interested to get notified about pyhash changes. The actual owners are @Haypo and @pitrou .

@pitrou
Copy link
Member

pitrou commented Jul 29, 2017

I really dislike the "owners" terminology as used here. There are "experts", which is the terminology used in the devguide. I personally don't want some people to start being possessive towards code they contributed (any more than they might already be).

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

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

I'm happy to be the guinea pig for your experiments. :)

@tiran
Copy link
Member

tiran commented Jul 29, 2017

Instead of assigning individual users to files, could we create teams and assign teams to files instead?

@pitrou yes, I agree with you. The term "owner" is problematic. However we can't do much about it. It's a github standard, https://help.github.com/articles/about-codeowners/

@Mariatta
Copy link
Member Author

Reading codeowner docs, it @org/team-name is supported, so we can already do:
@python/python-core, @python/asyncio, @python/typing.
If we go this path, we may need to create many new teams, and people will need to request to be added to/removed from tho team.
I think it's simpler to let individuals add/remove their usernames in codeowners, they can create the PR anytime.

@tiran
Copy link
Member

tiran commented Jul 29, 2017

@Mariatta the CODEOWNERS file may become hard to maintain pretty quickly. The file format has a last-match-wins ruling. Without teams it's going to be hard to maintain ownership for e.g. Docs/* for some and specific docs for others.

@brettcannon
Copy link
Member

@pitrou Yes, the terminology doesn't line up with what we will use the file for, but it's dictated by GitHub (they actually have an option to say a pull request can't be merged until an owner listed in that file has been cleared, hence the naming). We could have a comment at the top of the file to point this fact out if you want.

@tiran If we use a CODEOWNERS file then we're ditching mention-bot since the latter isn't working right now (not sure if you've noticed but it's actually been broken for well over a month now). Basically if we can rely on something that GitHub provides as a service rather than something Facebook provides just to be nice then I'll go with the one that has paid staffing to make sure it works. 😄 Otherwise we will need to stand up our own mention-bot instance and maintain it.

As for having teams, my plan was to get this file in first, and then solicit people to tell me what teams they wanted created as part of the announcement that the file now existed. I would then create the team and then make all team members admins for that team so they can control when they leave, adding others, etc. That means I only have to create the initial team and past that I don't have to maintain it. But one thing at a time. 😄

And I hope no one is silly enough to add a Docs/* rule as that would be extremely noisy. 😉

@Mariatta
Copy link
Member Author

I pushed the change. Please re-review.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

I don't understand the purpose of this file. Please write a short description maybe pointing to a full description somewhere else.

Does it technically prevent merging any kind of change in these files without @tiran approval?

I see a risk of abusing this file.

@vstinner
Copy link
Member

https://help.github.com/articles/about-codeowners/ mentions adding automatically requests reviews. That's fine.

My question is about the optional part: do you want to require @tiran review? Or later require someone else review on other parts of the code.

@Mariatta
Copy link
Member Author

In the current .mention-bot file, it has @tiran in "AlwaysNotifyForPaths" for these paths.
So I'm adding him here to CODEOWNERS for backward compatibility 😄

@Mariatta
Copy link
Member Author

I added some comments on top of the file.

@Mariatta
Copy link
Member Author

I think for now we are only notifying the owner/expert listed in codeowners file, but we are not requiring their review.

@Mariatta
Copy link
Member Author

Not sure if people want to be requested for review for backport PRs? I removed needs backport to <branch> labels.

@Mariatta
Copy link
Member Author

Mariatta commented Aug 1, 2017

😞 I feel terrible that I made two personal branches in python/cpython repo that lasted longer than 24 hours. So I'm going to merge this now 😛
And if the format/pattern in this file doesn't work out, please propose a new PR 😃 Thanks all!

@Mariatta Mariatta merged commit 00fce69 into master Aug 1, 2017
@Mariatta Mariatta deleted the Mariatta-codeowners branch August 1, 2017 03:56
@brettcannon
Copy link
Member

To answer @Haypo : it only blocks PRs if you check a certain box and I'm never going to check that box (trust me 😉 )

@vstinner
Copy link
Member

vstinner commented Aug 1, 2017 via email

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants