Everything

php

Sync CODEOWNERS

php.internals

Christoph Becker

2 years ago

Hi all!

I have noticed that the CODEOWNERS of php-src are apparently not synced
for all active branches[1][2].  However, it seems to me that this
doesn't make sense; if someone provides a PR against some lower branch,
the most recent CODEOWNERS may not be notified, and modifications might
be send to possibly inactive maintainers.  This is particularly
confusing if the base branch of an PR will be changed; all over sudden
new reviews may be requested.

So I suggest to sync CODEOWNERS for all active branches (and maybe even
security branches).

What do you think?

[1] <https://github.com/php/php-src/blob/PHP-8.2/.github/CODEOWNERS>
[2] <https://github.com/php/php-src/blob/master/.github/CODEOWNERS>

Cheers,
Christoph

Saki Takamachi

2 years ago

Hi Christoph,

> ﻿Hi all!
>
> I have noticed that the CODEOWNERS of php-src are apparently not synced
> for all active branches[1][2].  However, it seems to me that this
> doesn't make sense; if someone provides a PR against some lower branch,
> the most recent CODEOWNERS may not be notified, and modifications might
> be send to possibly inactive maintainers.  This is particularly
> confusing if the base branch of an PR will be changed; all over sudden
> new reviews may be requested.
>
> So I suggest to sync CODEOWNERS for all active branches (and maybe even
> security branches).
>
> What do you think?
>
> [1] <https://github.com/php/php-src/blob/PHP-8.2/.github/CODEOWNERS>
> [2] <https://github.com/php/php-src/blob/master/.github/CODEOWNERS>
>
> Cheers,
> Christoph

From what I can see, the difference is mostly me....
I'm in favor of syncing.

Regards,

Saki

Niels Dossche

2 years ago

On 18/07/2024 14:08, Christoph M. Becker wrote:

> Hi all!
>
> I have noticed that the CODEOWNERS of php-src are apparently not synced
> for all active branches[1][2].  However, it seems to me that this
> doesn't make sense; if someone provides a PR against some lower branch,
> the most recent CODEOWNERS may not be notified, and modifications might
> be send to possibly inactive maintainers.  This is particularly
> confusing if the base branch of an PR will be changed; all over sudden
> new reviews may be requested.
>
> So I suggest to sync CODEOWNERS for all active branches (and maybe even
> security branches).
>
> What do you think?
>
> [1] <https://github.com/php/php-src/blob/PHP-8.2/.github/CODEOWNERS>
> [2] <https://github.com/php/php-src/blob/master/.github/CODEOWNERS>
>
> Cheers,
> Christoph

Hi

I believe they should be synced indeed, although we should be careful
with extensions that are dropped from PHP (e.g. imap). Those entries
should remain only in 8.2 and 8.3 but not in master.

Kind regards
Niels

Christoph Becker

2 years ago

Hi Niels!

On 18.07.2024 at 14:44, Niels Dossche wrote:

> I believe they should be synced indeed, although we should be careful
> with extensions that are dropped from PHP (e.g. imap). Those entries
> should remain only in 8.2 and 8.3 but not in master.

Oh, right!  My idea was to handle this like bug fixes: target the lowest
branch which is relevant, and merge upwards as applicable (possibly do
empty merges up to `master`).

For those that may not have noticed yet: Peter Kokot provided PR #15017.
Maybe the details can be discussed there.

Cheers,
Christoph

Ilija Tovilo

2 years ago

Hi Christoph

On Thu, Jul 18, 2024 at 2:09 PM Christoph M. Becker <cmbecker69@gmx.de> wrote:

>
> So I suggest to sync CODEOWNERS for all active branches (and maybe even
> security branches).
>
> What do you think?

I think back when it was introduced it wasn't clear (at least to me)
that PRs would request reviews based on the CODEOWNERS file of the
target branch, rather than the default branch. Given that's how it
works, syncing it in all active branches makes sense to me.

Ilija

Peter Kokot

2 years ago

On Thu, 18 Jul 2024, 16:32 Ilija Tovilo, <tovilo.ilija@gmail.com> wrote:

> Hi Christoph
>
> On Thu, Jul 18, 2024 at 2:09 PM Christoph M. Becker <cmbecker69@gmx.de>
> wrote:
> >
> > So I suggest to sync CODEOWNERS for all active branches (and maybe even
> > security branches).
> >
> > What do you think?
>
> I think back when it was introduced it wasn't clear (at least to me)
> that PRs would request reviews based on the CODEOWNERS file of the
> target branch, rather than the default branch. Given that's how it
> works, syncing it in all active branches makes sense to me.
>
> Ilija
>

I think that nothing will help solve this issue but to write also some
comment in that file on top to send updates to the earliest supported
branch, and not the master branch only.

We were resolving this in the past not so long ago:
https://github.com/php/php-src/pull/13591