Machine-scope maintainers for OpenBMC

[Andrew Jeffery]

On Wed, 2017-10-18 at 16:57 -0700, brendanhiggins at google.com wrote:
> > On Tue, 2017-10-17 at 11:25 -0700, brendanhiggins at google.com wrote:
> > > > Hello!
> > > > 
> > > > I'm wondering whether we should jot down the maintainers of machines
> > > > supported by OpenBMC, and if so, where? With respect to the latter, one
> > > > candidate might be the MAINTAINERS file in openbmc/docs.
> > > 
> > > I agree with that idea; this is what the MAINTAINERS file was supposed to be
> > > for. The reason I chose the syntax @repo-name://*, was that the '*' at the end
> > > is supposed to be a file matcher path (this is mentioned in the header of the
> > > document); however, at the time there were no sub-repo owners, hence all the
> > > entries were @repo-name://*.
> > 
> > Indeed! Given it's a per-repo description it may get a bit unwieldy but we'll
> > see how it turns out. Primarily, my concern is that we will have to at least
> > describe paths in the kernel repo, paths in the openbmc/openbmc repo, and
> > possibly a bunch of others. I'm curious if there isn't a more effective way to
> > resolve this, but there's probably not as that's how the code is structured
> > (and if we want to have something like get_maintainers.pl, then it needs to
> > know about these paths).
> 
> True, maybe there will be some way to shard the file in the future? If we only
> have the MAINTAINERS file at the root of each repo, it still does not need to
> know much about the code structure, but we loose the ability to have one
> MAINTAINERS file that describes cross repository machine ownership, which might
> be valuable. Time might provide the best insight on this.

Yeah, agreed.

> 
> > 
> > > 
> > > Nancy and I talked to Brad yesterday and he mentioned that he would like to have
> > > a way to enforce sub-repo permissions, since Gerrit only enforces permissions on
> > > a repo level. We, Google, have recently started doing automated reviews to
> > > verify that Google internal changes had owners that we could go back to durring
> > > a merge/rebase.
> > 
> > Can you expand on what you are talking about here? You've piqued my interest.
> 
> Sure, I wrote a Gerrit bot that looks at the commit messages on changes
> submitted to Gerrit. This bot, which I named Gerrit Watcher, checks that the
> footers, discussed in my linux-change-tracking doc, are present and that the
> commit is appropriately maintained by someone.
> 
> If the change is not "Upstream" or "In-review" (also discussed in the doc),
> Gerrit Watcher checks that all affected files are listed in Google's MAINTAINERS
> file (with some exceptions). This is logically similar to what we would want to
> do here, since we would be scanning our @docs://MAINTAINERS file for matching
> entries and then adding those people to the review and then want to make sure
> that they actually had a chance to do the review.
> 
> > 
> > > It should be pretty straightforward to do something similar for
> > > enforcing sub-repo reviewers/maintainers: we could have a bot that would listen
> > > for all new changes (similar to the Jenkins bot) and then add the appropriate
> > > reviewers based on the MAINTAINER file entries; it could also then vote +1/+2
> > > once one or more of the appropriate reviewers have signed off.
> > 
> > Right. There are gerrit plugins that can auto-assign reviewers based on
> > git-blame and such, maybe we could also massage one of those. Is there much
> > benefit in the bot assigning a score?
> 
> Having the bot assign a score is just provides a zero effort way to see that the
> change has gotten the appropriate reviews. We might not require that all
> reviewers sign-off on a change, but only the people listed in
> @docs://MAINTAINERS, or maybe even a subset of them; that might not be entirely
> obvious to whoever is responsible for merging the change.

Ah, right. Makes sense.

> 
> Just a thought. Maybe it is better to just make sure people are keeping up with
> who is looking at the reviews. Or maybe we should be more liberal with who gets
> to +2.
> 
> > 
> > > 
> > > Gerrit documentation for doing this type of thing can be found here:
> > > https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html
> > > 
> > > > 
> > > > A related question is: Who are the machine maintainers? Off the top of
> > > > my head I feel the following machines machines match with people:
> > > > 
> > > >     meta-evb/meta-evb-raspberrypi                  Yi TZ Li <shliyi at cn.ibm.com>
> > > >     meta-evb/meta-evb-aspeed/meta-evb-ast2500      Joel Stanley <joel at jms.id.au>
> > > >     meta-x86/meta-mellanox/meta-msn                Mykola Kostenok <c_mykolak at mellanox.com>
> > > >     meta-x86/meta-quanta/meta-q71l                 Patrick Venture <venture at google.com>    , Rick Altherr <raltherr at google.com>
> > > >     meta-openpower/meta-ingrasys/meta-zaius        Xo Wang <xow at google.com>
> > > >     meta-openpower/meta-ibm/meta-firestone         Lei YU <mine260309 at gmail.com>
> > > >     meta-openpower/meta-ibm/meta-romulus           Lei YU <mine260309 at gmail.com>
> > > >     meta-openpower/meta-ibm/meta-witherspoon       Brad Bishop <bradleyb at fuzziesquirrel.com>, Andrew Geissler <geissonator at gmail.com>
> > > 
> > > I proposed a change that adds the above entries in the syntax that I suggested
> > > above: https://gerrit.openbmc-project.xyz/#/c/7409/
> > 
> > Thanks, I'll take a look.
> > 
> > > 
> > > > 
> > > > Less obvious are the following machines:
> > > > 
> > > >     meta-openpower/meta-ibm/meta-palmetto
> > > >     meta-openpower/meta-ibm/meta-garrison
> > > >     meta-openpower/meta-rackspace/meta-barreleye
> > > > 
> > > > Yet another related question is what do we want to do with machines
> > > > that don't have an obvious maintainer?
> > > > 
> > > > In the case of Palmetto I imagine we could find someone willing to step
> > > > up (/me ducks). Garrison I'm unsure about, and for Barreleye I've had
> > > > kernel patches on the list for several months with not a lot of
> > > > interest in them, which indicates to me that we could probably drop it.
> > > > 
> > > > As some background, Joel and I were recently discussing machine
> > > > maintainers with respect to the dev-4.13 kernel effort. The approach
> > > > Joel kicked off for dev-4.13 was to start from scratch against upstream
> > > > and distribute the forward-porting amongst owners of patches in dev-
> > > > 4.10. That is, if you have patches in dev-4.10 and that are not in
> > > > upstream v4.13, you should rework and send them for us to apply to dev-
> > > > 4.13.
> > > 
> > > So this is something we have been trying to deal with internally as well, I came
> > > up with a merge/rebase document that described a system for keeping track of
> > > internal changes. At its core, it relies on a set of commit footers:
> > > 
> > > * Effort - used to specify who the work is for. This makes the project
> > >   responsible for supplying people to address problems supporting these commits.
> > > * Origin - Specifies the original version of an internal commit; used when an
> > >   internal commit is rebased or cherry-picked.
> > > * Dropped - Specifies that a commit is to be dropped in any rebase.
> > > * Upstream - Specifies that a commit is to be dropped if the commit can be found
> > >   in the branch being rebased to.
> > > * Git-repo - used in conjunction with Upstream, used to specify a non-mainline
> > >   upstream repository, this should probably always be an upstream maintainer's
> > >   for-next repository.
> > > * In-review - used to specify where the most recent version of a patch is being
> > >   reviewed upstream.
> > > * Maintainer - used in exceptionally rare circumstances to identify who is
> > >   responsible for maintaining an internal patch.
> > > 
> > > We also have an internal GOOGLE_MAINTAINERS file for files that are not
> > > upstream, rather than using the Maintainer footer for everything.
> > > 
> > > The idea is, that whenever we do a rebase, we can run a script that looks at the
> > > above footers and should be able to resolve most of the issues; either by
> > > automatically dropping the commit, when it has been superceded, or contacting
> > > the appropriate owner when the issue needs to be resolved manually.
> > > 
> > > Let me know if this sounds useful, and I will get the document in a format that
> > > I can share via email.
> > 
> > I'd certainly like to explore this more, so if you can share please do! I'd
> > like to get feedback from people on the list (primarily those affected,
> > particularly Joel, and I expect Brad will be interested).
> 
> Cool, originally I was going to scrub our internal change tracking doc and
> attach it below as markdown, but then I decided that it would not take much more
> work to make it OpenBMC specific, so I just went ahead and did it. I posted it
> > as an RFC here: https://gerrit.openbmc-project.xyz/#/c/7435/. Let me know what
> you think!
> 

I intend to get to reviewing this shortly.

Cheers,

Andrew
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20171023/ade07495/attachment.sig>