Approvals for Critical Changes

[Gregory Nutt <sp...@gmail.com>]

In the past, we talked about not all changes being equal and that 
changes within some directories are more sensitive than changes to 
others.  We discussed the notion of an "owner" of each top level 
directory that needed to approve any changes to those directories.  
Nathan was the primary advocate for the concept.  I think that it is 
time that we re-consider this idea.

Is there any github way to automate this so that the cognizant person's 
approval is required before any change to a directory?  I suspect not 
since that is a pretty specific requirement.

As the number of committers increases, I think there is a real changes 
of ill-informed, ill-conceived, under-reviewed changes to make into 
critical parts of the OS.  How do we protect against that?  We need to 
assure that the proper reviews and approvals are in place for changes to 
those critical parts.

Greg

Re: Approvals for Critical Changes

[Gregory Nutt <sp...@gmail.com>]

> One thing we can do is automatically apply labels to pull requests via the
> CI system. So if something changes in certain folder gets a multiple
> reviewer label and a comment in the PR.
>
> Still a social solution, but I think it would nudge people into the right
> direction.
I like the idea.

Re: Approvals for Critical Changes

[Brennan Ashton <ba...@brennanashton.com>]

One thing we can do is automatically apply labels to pull requests via the
CI system. So if something changes in certain folder gets a multiple
reviewer label and a comment in the PR.

Still a social solution, but I think it would nudge people into the right
direction.

--Brennan

On Mon, Apr 6, 2020, 4:04 PM Gregory Nutt <sp...@gmail.com> wrote:

>
> > The only project I know that does something similar (successfully) is
> subversion and it's by social convention.
>
> Social conventions would be the preferred way to handle this. But that
> requires some education for all committers to understand and support
> those conventions.  That can be difficult with new people coming on board.
>
>
>

Re: Approvals for Critical Changes

[Gregory Nutt <sp...@gmail.com>]

> The only project I know that does something similar (successfully) is subversion and it's by social convention.

Social conventions would be the preferred way to handle this. But that 
requires some education for all committers to understand and support 
those conventions.  That can be difficult with new people coming on board.

Re: Approvals for Critical Changes

[Adam Feuer <ad...@starcat.io>]

Justin,

Thanks, that's cool to know.

-adam

On Mon, Apr 6, 2020 at 3:43 PM Justin Mclean <ju...@classsoftware.com>
wrote:

> Hi,
>
> The only project I know that does something similar (successfully) is
> subversion and it's by social convention.
>
> Please consider not using CODEOWNERS as people don’t own the parts of the
> code at the ASF. It can also cause long term issues and discourage
> contribution.Tjhere are other issues, for instance, what happens if a code
> owner is no longer involved in a project? At least carefully consider it in
> the ASF content before doing so. One other incubating project (since
> graduating) is using it, they were asked not to, but they did continue to
> do so but added the equivalent of this to make it clear who owns the code.
>
> # Global owners
> * @apache/nuttx-committers
>
> Thanks,
> Justin
>
>

-- 
Adam Feuer <ad...@starcat.io>

Re: Approvals for Critical Changes

[Justin Mclean <ju...@classsoftware.com>]

Hi,

The only project I know that does something similar (successfully) is subversion and it's by social convention.

Please consider not using CODEOWNERS as people don’t own the parts of the code at the ASF. It can also cause long term issues and discourage contribution.Tjhere are other issues, for instance, what happens if a code owner is no longer involved in a project? At least carefully consider it in the ASF content before doing so. One other incubating project (since graduating) is using it, they were asked not to, but they did continue to do so but added the equivalent of this to make it clear who owns the code.

# Global owners
* @apache/nuttx-committers

Thanks,
Justin

Re: Approvals for Critical Changes

[Adam Feuer <ad...@starcat.io>]

Greg,

Github appears to have Code Owners:

"You can use a CODEOWNERS file to define individuals or teams that are
responsible for code in a repository."
"Code owners are automatically requested for review when someone opens a
pull request that modifies code that they own. Code owners are not
automatically requested to review draft pull requests."

https://help.github.com/en/github/creating-cloning-and-archiving-repositories/about-code-owners

Is that what you're looking for?

cheers
adam


On Mon, Apr 6, 2020 at 1:47 PM Gregory Nutt <sp...@gmail.com> wrote:

> In the past, we talked about not all changes being equal and that
> changes within some directories are more sensitive than changes to
> others.  We discussed the notion of an "owner" of each top level
> directory that needed to approve any changes to those directories.
> Nathan was the primary advocate for the concept.  I think that it is
> time that we re-consider this idea.
>
> Is there any github way to automate this so that the cognizant person's
> approval is required before any change to a directory?  I suspect not
> since that is a pretty specific requirement.
>
> As the number of committers increases, I think there is a real changes
> of ill-informed, ill-conceived, under-reviewed changes to make into
> critical parts of the OS.  How do we protect against that?  We need to
> assure that the proper reviews and approvals are in place for changes to
> those critical parts.
>
> Greg
>
>
>

-- 
Adam Feuer <ad...@starcat.io>

Re: Approvals for Critical Changes

[Gregory Nutt <sp...@gmail.com>]

>> Just to clarify:
>>
>> I don't support that specific people should "own" a directory or
>> section of code,
>
> Poor choice of words on my part.  I seem to pick words that trigger 
> the wrong responses.  We are in agreement int the concept.  By "own", 
> I meant in the sense that businesses use the term:  When a business 
> encourages you to take ownership of code, they a referring only to 
> taking responsibility for the integrity of a body of code.  Certainly 
> businesses don't think you really own anything.

Ultimately, what we need is for the right eyes to look at the code 
before it is committed. People who do not have a deep understanding of a 
section of code should defer the review and merge to other people who do 
have that understanding.  That is a courtesy and not required by any 
tool.  It is just good responsible behavior

For example, whenever I see modifications to STM32, i.MXRT, or Kinetis I 
automatically select DavidS as the reviewer.  He/PX4 has a vested 
interest as well as deep understanding of those architectures.  
Similarly, I would hope that any changes under sched would include me as 
a reviewer.

Re: Approvals for Critical Changes

[Nathan Hartman <ha...@gmail.com>]

On Mon, Apr 6, 2020 at 8:23 PM Gregory Nutt <sp...@gmail.com> wrote:

>
> > Just to clarify:
> >
> > I don't support that specific people should "own" a directory or
> > section of code,
>
> Poor choice of words on my part.  I seem to pick words that trigger the
> wrong responses.


I didn't take it the wrong way. No worries. :-)

Cheers,
Nathan

Re: Approvals for Critical Changes

[Gregory Nutt <sp...@gmail.com>]

> Just to clarify:
>
> I don't support that specific people should "own" a directory or
> section of code,

Poor choice of words on my part.  I seem to pick words that trigger the 
wrong responses.  We are in agreement int the concept.  By "own", I 
meant in the sense that businesses use the term:  When a business 
encourages you to take ownership of code, they a referring only to 
taking responsibility for the integrity of a body of code.  Certainly 
businesses don't think you really own anything.

Re: Approvals for Critical Changes

[Nathan Hartman <ha...@gmail.com>]

On Mon, Apr 6, 2020 at 4:47 PM Gregory Nutt <sp...@gmail.com> wrote:
> In the past, we talked about not all changes being equal and that
> changes within some directories are more sensitive than changes to
> others.  We discussed the notion of an "owner" of each top level
> directory that needed to approve any changes to those directories.
> Nathan was the primary advocate for the concept.  I think that it is
> time that we re-consider this idea.

Just to clarify:

I don't support that specific people should "own" a directory or
section of code, but what I do support is the idea that certain areas
are more critical and need more people to give an OK before merging
changes that affect those areas; the individual committers are
expected to "know what they don't know" and not weigh in on approving
things they don't understand.

There's the concept of an "obvious fix" that doesn't apply: Anyone who
wants to fix typos, grammar, wrong/misleading comments, non-functional
changes, etc., should be able to do so without requiring the same
scrutiny as functional changes.

Regarding what Justin said about Subversion:

On Mon, Apr 6, 2020 at 6:43 PM Justin Mclean <ju...@classsoftware.com> wrote:
> The only project I know that does something similar (successfully) is subversion and it's by social convention.

I happen to be on the Subversion PMC and AFAIK no one "owns" any
section of anything; in fact, our Community Guide (a.k.a. "HACKING")
has this to say:

"We have a tradition of not marking files with the names of individual
authors (i.e., we don't put lines like "Author: foo" or "@author foo"
in a special position at the top of a source file). This is to
discourage territoriality — even when a file has only one author, we
want to make sure others feel free to make changes. People might be
unnecessarily hesitant if someone appears to have staked a personal
claim to the file."

Again, you're expected to "know what you don't know" and it's on the
honor system. Of course, since we (and our employers) all keep
critical code in Subversion repositories, none of us wants to mess it
up!! :-)

So, bottom line: I support the idea that critical areas should require
more review, but I don't support "owners" of files.

Cheers,
Nathan