Skip to content

Add a CODEOWNERS file #8670

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

Closed
wants to merge 1 commit into from
Closed

Add a CODEOWNERS file #8670

wants to merge 1 commit into from

Conversation

ramsey
Copy link
Member

@ramsey ramsey commented May 31, 2022

I'm using this pull request as a point of discussion and to get feedback from the internals community.

As release managers go through open PRs, tagging them with "Waiting on Review," it would be helpful to know from whom we should request reviews, based on the area of the code. One way is to set up a table on the wiki with this information. Another way is to use a GitHub CODEOWNERS file, which takes care of requesting the reviews automatically.

I don't know how feasible it is to maintain a list like this for the PHP project. Ideally, folks would add themselves to the list and self-identify which areas of the code base they want to have automatically request them for reviews.


Apologies to @derickr and @cjbj for using them as examples in this PR. 😄

@ramsey
Copy link
Member Author

ramsey commented May 31, 2022

Discussing on the mailing list here: https://externals.io/message/117843

@Girgias
Copy link
Member

Girgias commented May 31, 2022

In theory https://github.com/php/php-src/blob/master/EXTENSIONS is meant to track who's responsible of what

@ramsey
Copy link
Member Author

ramsey commented May 31, 2022

In theory https://github.com/php/php-src/blob/master/EXTENSIONS is meant to track who's responsible of what

🤦🏻‍♂️ Maybe I should have asked before opening this PR and mailing the list

@Girgias
Copy link
Member

Girgias commented May 31, 2022

In theory https://github.com/php/php-src/blob/master/EXTENSIONS is meant to track who's responsible of what

🤦🏻‍♂️ Maybe I should have asked before opening this PR and mailing the list

It might still make sense to use a CODEOWNERS file for the GitHub integration, I suppose it automatically pings the relevant person if a PR is made against those files?

@ramsey
Copy link
Member Author

ramsey commented May 31, 2022

I suppose it automatically pings the relevant person if a PR is made against those files?

Yes. It automatically requests a review from them, and they get a notification according to their notification settings.

@saundefined
Copy link
Member

@ramsey Also, I guess we can automate the PRs labeling for extensions.

For example, with actions/labeler action.

What do you think?

@ramsey
Copy link
Member Author

ramsey commented Jun 2, 2022

I guess we can automate the PRs labeling for extensions.

Oh, that looks good!

@saundefined
Copy link
Member

Oh, that looks good!

Tried it now, looks good =)

Demo: https://github.com/saundefined/php-src/pull/2

@ramsey
Copy link
Member Author

ramsey commented Jun 2, 2022

I like it. That needs less coordination among internals folks than the CODEOWNERS, I think. Open a PR, and we can merge it in. Thanks!

@saundefined saundefined mentioned this pull request Jun 3, 2022
@MCMic
Copy link
Contributor

MCMic commented Jun 4, 2022

How is the EXTENSIONS file maintained? Where does the end year comes from for maintainers? I'm listed as maintainer of ldap up until 2017, but as far as I know I still am. Also, I'd like to update my surname if possible.

@Girgias
Copy link
Member

Girgias commented Jun 4, 2022

How is the EXTENSIONS file maintained? Where does the end year comes from for maintainers? I'm listed as maintainer of ldap up until 2017, but as far as I know I still am. Also, I'd like to update my surname if possible.

People should update the file if they want to extend the maintenance year, so it's a very manual and forgetful process

@php php deleted a comment from rwema3 Jun 8, 2022
@cmb69
Copy link
Member

cmb69 commented Dec 9, 2022

In my opinion, it makes sense to have a CODEOWNERS file (although I don't like the term "code owner"; nobody owns any part of php-src). Contrary to EXTENSIONS, CODEOWNERS has some value, and it might be more realistic to hope that it is kept up to date. In the long run, CODEOWNERS might replace EXTENSIONS (although that should be discussed on the internals mailing list then).

Comment on lines +12 to +13
/ext/oci8/ @cjbj
/ext/pdo_oci/ @cjbj
Copy link
Member

@cmb69 cmb69 Dec 9, 2022

Choose a reason for hiding this comment

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

I don't think we should list people without their explicit consent (@derickr already said he likes to be informed about ext/date issues), except maybe for those five maintainers in EXTENSIONS which are listed for 2022.

Suggested change
/ext/oci8/ @cjbj
/ext/pdo_oci/ @cjbj
/ext/com_dotnet/ @cmb69
/ext/gd/ @cmb69
/ext/oci8/ @cjbj
/ext/pdo_oci/ @cjbj

Copy link
Contributor

Choose a reason for hiding this comment

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

You can list me if it makes sense. My affiliation is not a secret !

Copy link
Member

Choose a reason for hiding this comment

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

Updated the suggestion above.

@TimWolla
Copy link
Member

TimWolla commented Dec 9, 2022

As also just mentioned in R11, I'm also interested in this for /ext/random/ @TimWolla.

@Girgias
Copy link
Member

Girgias commented Dec 12, 2022

As I've been touching DBA the latest and kinda know how it works, I don't mind being marked for the DBA extension.
In theory, I could also be put on the ext-imap one, but I can't compile it easily any more on Fedora since c-client got removed.

@cmb69
Copy link
Member

cmb69 commented Dec 12, 2022

I can't compile it easily any more on Fedora since c-client got removed.

OT, but an additional reason to move ext/imap to PECL soon.

@iluuu1994
Copy link
Member

I'm also in favor of this. I don't watch the php-src repository because it is way too noisy. This would allow me to subscribe to the PRs that I may actually be helpful for.

@iluuu1994
Copy link
Member

I think to avoid too many discussions the empty file should be added. Anybody who's interested in being associated with a part of PHP can add themselves.

@TimWolla
Copy link
Member

I agree with "someone make the start", as multiple folks expressed their interest, otherwise this is likely not going to go anywhere.

@iluuu1994 iluuu1994 closed this in f89a67e Mar 20, 2023
@iluuu1994
Copy link
Member

I took the liberty of merging with the changes discussed. Anybody interested can create a PR or add themselves directly to the file.

@ramsey
Copy link
Member Author

ramsey commented Mar 20, 2023

Thanks, @iluuu1994!

@ramsey ramsey deleted the github-codeowners branch March 20, 2023 18:07
@petk petk mentioned this pull request Mar 4, 2024
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.

8 participants