Machine-scope maintainers for OpenBMC
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.
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 < [email protected] > meta-evb/meta-evb-aspeed/meta-evb-ast2500 Joel Stanley < [email protected] > meta-x86/meta-mellanox/meta-msn Mykola Kostenok < [email protected] > meta-x86/meta-quanta/meta-q71l Patrick Venture < [email protected]> , Rick Altherr < [email protected] > meta-openpower/meta-ingrasys/meta-zaius Xo Wang < [email protected] >
meta-openpower/meta-ibm/meta-firestone Lei YU < [email protected]> meta-openpower/meta-ibm/meta-romulus Lei YU < [email protected]>
meta-openpower/meta-ibm/meta-witherspoon Brad Bishop <[email protected]>, Andrew Geissler <[email protected]>
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.
To stress the point a bit, the dev-4.13 tree as it stands does not have niceties such as arch/arm/mach-aspeed/aspeed.c or any of the machine- specific devicetree files. The idea is that in addition to distributing the workload, we only add back in what we need and try to avoid carrying accumulated cruft across moves to new upstream releases.
Whilst the distributed approach eases the workload on Joel, it does mean that we could miss things that are required for specific machines (the board-file hacks are particularly worrisome). This is driving my interest in identifying the maintainers. Having such a list of people would, for instance, clarify who ought to be on the review list for kernel bumps to openbmc/openbmc in Gerrit. In the case of dev-4.13 this would help with the go/no-go decision.
So if you have thoughts on the above or want to put your hand up to claim any of the machines in the limbo list above, please do speak up!
Cheers,
Andrew
Hi Andrew, Brad,
I would like to bring a question about how do we welcome system vendors' systems.
Background: in gerrit we already have some system vendors' machines under review:
* meta-inventec/meta-lanyang: https://gerrit.openbmc-project.xyz/#/c/3888/
* meta-yadro/meta-vesnin: https://gerrit.openbmc-project.xyz/#/c/6429/
They both contain a "large" commit that introduce a machine.
I know we have suggestions to split the commit into separated smaller ones, but this
seems a bit hard:
* The work is based (referenced) on a similar machine, e.g palmetto or zaius, which
means the base code (meta-xxx) is already big;
* The change is submitted as a "squashed" one after it's built and tested, which means
not each "internal" commit can be built successfully.
In such case, how do we handle this case? Is it possible to accept a large squashed commit
if and *only* if the change is to introduce a new meta-machine?
Thanks!
Quoted 59 lines · Andrew Jeffery
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
MAINTAINERSfile in openbmc/docs.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 < [email protected] > meta-evb/meta-evb-aspeed/meta-evb-ast2500 Joel Stanley < [email protected] > meta-x86/meta-mellanox/meta-msn Mykola Kostenok < [email protected] > meta-x86/meta-quanta/meta-q71l Patrick Venture < [email protected]> , Rick Altherr < [email protected] > meta-openpower/meta-ingrasys/meta-zaius Xo Wang < [email protected] > meta-openpower/meta-ibm/meta-firestone Lei YU < [email protected]> meta-openpower/meta-ibm/meta-romulus Lei YU < [email protected]>meta-openpower/meta-ibm/meta-witherspoon Brad Bishop <[email protected]>, Andrew Geissler <[email protected]>
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
To stress the point a bit, the dev-4.13 tree as it stands does not have niceties such as
arch/arm/mach-aspeed/aspeed.cor any of the machine- specific devicetree files. The idea is that in addition to distributing the workload, we only add back in what we need and try to avoid carrying accumulated cruft across moves to new upstream releases.Whilst the distributed approach eases the workload on Joel, it does mean that we could miss things that are required for specific machines (the board-file hacks are particularly worrisome). This is driving my interest in identifying the maintainers. Having such a list of people would, for instance, clarify who ought to be on the review list for kernel bumps to openbmc/openbmc in Gerrit. In the case of dev-4.13 this would help with the go/no-go decision.
So if you have thoughts on the above or want to put your hand up to claim any of the machines in the limbo list above, please do speak up!
Cheers,
Andrew
Quoted 22 lines · Lei YU
Hi Andrew, Brad,
I would like to bring a question about how do we welcome system vendors' systems.
Background: in gerrit we already have some system vendors' machines under review:
* meta-inventec/meta-lanyang: https://gerrit.openbmc-project.xyz/#/c/3888/
* meta-yadro/meta-vesnin: https://gerrit.openbmc-project.xyz/#/c/6429/They both contain a "large" commit that introduce a machine.
I know we have suggestions to split the commit into separated smaller ones, but this
seems a bit hard:
* The work is based (referenced) on a similar machine, e.g palmetto or zaius, which
means the base code (meta-xxx) is already big;
* The change is submitted as a "squashed" one after it's built and tested, which means
not each "internal" commit can be built successfully.
Quoted 3 lines
In such case, how do we handle this case? Is it possible to accept a large squashed commit
if and *only* if the change is to introduce a new meta-machine?
I think that depends. What are you comfortable reviewing? Personally I *much* prefer small, obvious patches than a hulking diff in the magnitude of thousands of lines. I find it *much* harder to track what's going on in the latter type of change. Bear in mind that this is code that could/will be executing on any number of people's machines, and we have a responsibility to the community to ensure it's not doing anything too crazy.
If you're comfortable reviewing large patches like that and signing off on them then you may have just won yourself a lot of work :D Otherwise, my view is contributors should help themselves by helping those reviewing the code say yes to their contribution, which means at least self-contained changes which are hopefully small.
However, we do run into one of my favourite topics: essential vs accidental complexity[1]. Sometimes it's not possible to break things down further. In this case reviewers might have to just deal with large patches, but contributors should be willing to bend to their judgement on whether the patch has been reduced to deal with only the essential complexity of the problem. If it hasn't, the patch should probably be split.
I'm hesitant to provide a blanket rule like "all new system additions can be a patch bomb" without ruling out accidental complexity from the problem.
[1] https://en.wikipedia.org/wiki/No_Silver_Bullet
Sure, this all raises the bar to contribution a bit. However I feel it does so in a way that leads to positive long-term contributions to the community by demonstrating some commitment, rather than just having code dumped over the wall with the risk that it gets neglected.
Maybe this could be seen as being exclusive. That's true to an extent, but I feel it's principled exclusion: That we should weight exclusion for the quality of the implementation of the contribution (i.e. whether the patch can be understood with confidence, the quality of the design, quality of the code code, whether it has tests, etc), but we should heavily weight *inclusion* for the requirements and concepts driving a contribution (adding support for previously unsupported systems, alternative implementations of DBus interfaces if they meet new requirements, etc).
To clarify, saying no to a patch adding a new system should probably not be interpreted as "no we don't want to support your system", rather "thanks for your effort to contribute support for your system! However I think you can help us help you by arranging the patch differently".
To finish, these are my personal opinions on the matter, and may not be reflected by the community at large. I'm keen to hear what other people think. Hopefully it addressed your query somewhere in the wall of text :)
Andrew
Quoted 16 lines · Andrew Jeffery
Quoted 12 lines · Lei YU
Hi Andrew, Brad,
I would like to bring a question about how do we welcome system vendors' systems.
Background: in gerrit we already have some system vendors' machines under review:
* meta-inventec/meta-lanyang: https://gerrit.openbmc-project.xyz/#/c/3888/
* meta-yadro/meta-vesnin: https://gerrit.openbmc-project.xyz/#/c/6429/They both contain a "large" commit that introduce a machine.
Quoted 9 lines
Quoted 9 lines
I know we have suggestions to split the commit into separated smaller ones, but this
seems a bit hard:
* The work is based (referenced) on a similar machine, e.g palmetto or zaius, which
means the base code (meta-xxx) is already big;
* The change is submitted as a "squashed" one after it's built and tested, which means
not each "internal" commit can be built successfully.
I understand. I remember the first time someone asked me to split a bunch of my work into small pieces. My first thought was, why!?! It works as-is.
But I think this view is just an artifact of not having your code peer reviewed (or not properly) in the past, or having to do peer review yourself. (I’m not referring to you personally Lei, just in general).
It really doesn’t take very long participating in an environment where peer review is primary to understand the benefit for both the author and the reviewer, of that extra effort.
Quoted 4 lines
Quoted 4 lines
In such case, how do we handle this case? Is it possible to accept a large squashed commit
if and *only* if the change is to introduce a new meta-machine?
For the new machine case, we may have an opportunity to ease the transition for new contributors with a new machine cookbook or wizard… one that includes help constructing a series of patches rather than just one.
Quoted 49 lines
I think that depends. What are you comfortable reviewing? Personally I *much* prefer small, obvious patches than a hulking diff in the magnitude of thousands of lines. I find it *much* harder to track what's going on in the latter type of change. Bear in mind that this is code that could/will be executing on any number of people's machines, and we have a responsibility to the community to ensure it's not doing anything too crazy.
If you're comfortable reviewing large patches like that and signing off on them then you may have just won yourself a lot of work :D Otherwise, my view is contributors should help themselves by helping those reviewing the code say yes to their contribution, which means at least self-contained changes which are hopefully small.
However, we do run into one of my favourite topics: essential vs accidental complexity[1]. Sometimes it's not possible to break things down further. In this case reviewers might have to just deal with large patches, but contributors should be willing to bend to their judgement on whether the patch has been reduced to deal with only the essential complexity of the problem. If it hasn't, the patch should probably be split.
I'm hesitant to provide a blanket rule like "all new system additions can be a patch bomb" without ruling out accidental complexity from the problem.
[1] https://en.wikipedia.org/wiki/No_Silver_Bullet
Sure, this all raises the bar to contribution a bit. However I feel it does so in a way that leads to positive long-term contributions to the community by demonstrating some commitment, rather than just having code dumped over the wall with the risk that it gets neglected.
Maybe this could be seen as being exclusive. That's true to an extent, but I feel it's principled exclusion: That we should weight exclusion for the quality of the implementation of the contribution (i.e. whether the patch can be understood with confidence, the quality of the design, quality of the code code, whether it has tests, etc), but we should heavily weight *inclusion* for the requirements and concepts driving a contribution (adding support for previously unsupported systems, alternative implementations of DBus interfaces if they meet new requirements, etc).
To clarify, saying no to a patch adding a new system should probably not be interpreted as "no we don't want to support your system", rather "thanks for your effort to contribute support for your system! However I think you can help us help you by arranging the patch differently".
Quoted 6 lines
To finish, these are my personal opinions on the matter, and may not be reflected by the community at large. I'm keen to hear what other people think. Hopefully it addressed your query somewhere in the wall of text :)
Andrew
I’m pretty much in complete agreement with all of Andrew’s points.
-brad
Quoted 5 lines
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
MAINTAINERSfile 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://*.
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. 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.
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html
Quoted 11 lines
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/
Quoted 15 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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.
Quoted 15 lines
To stress the point a bit, the dev-4.13 tree as it stands does not have niceties such as
arch/arm/mach-aspeed/aspeed.cor any of the machine- specific devicetree files. The idea is that in addition to distributing the workload, we only add back in what we need and try to avoid carrying accumulated cruft across moves to new upstream releases.Whilst the distributed approach eases the workload on Joel, it does mean that we could miss things that are required for specific machines (the board-file hacks are particularly worrisome). This is driving my interest in identifying the maintainers. Having such a list of people would, for instance, clarify who ought to be on the review list for kernel bumps to openbmc/openbmc in Gerrit. In the case of dev-4.13 this would help with the go/no-go decision.
Quoted 6 lines
So if you have thoughts on the above or want to put your hand up to claim any of the machines in the limbo list above, please do speak up!
Cheers,
Andrew
Cheers
Quoted 11 lines · On Tue, 2017-10-17 at 11:25 -0700, [email protected] wrote:
Quoted 5 lines
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 theMAINTAINERSfile in openbmc/docs.
I agree with that idea; this is what theMAINTAINERSfile 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).
Quoted 6 lines
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.
Quoted 5 lines
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?
Quoted 16 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:Quoted 8 lines
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_mykolakat 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>
Quoted 3 lines
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.
Quoted 45 lines
Quoted 14 lines
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.Quoted 8 lines
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 internalGOOGLE_MAINTAINERSfile for files that are not upstream, rather than using the Maintainer footer for everything.
Quoted 8 lines
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).
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!
Sorry for the spam. I was messing around with sendmail on my machine. I thought the queue was broken, apparently it wasn't.
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!
Quoted 21 lines · On Wed, 2017-10-18 at 16:57 -0700, [email protected] wrote:
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.Quoted 7 lines
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).
Quoted 5 lines
True, maybe there will be some way to shard the file in the future? If we only have the
MAINTAINERSfile 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 oneMAINTAINERSfile that describes cross repository machine ownership, which might be valuable. Time might provide the best insight on this.
Yeah, agreed.
Quoted 34 lines
Quoted 10 lines
Quoted 8 lines
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
MAINTAINERSfile (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.Quoted 10 lines
Quoted 6 lines
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?
Quoted 5 lines
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.
Quoted 86 lines
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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_mykolakat 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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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).
Quoted 6 lines
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
Quoted 1 line
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
Quoted 13 lines · brendanhiggins at google.com
Quoted 11 lines
Quoted 5 lines
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
MAINTAINERSfile in openbmc/docs.I agree with that idea; this is what the
MAINTAINERSfile 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://*.
Quoted 7 lines
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.
Quoted 9 lines
Quoted 7 lines
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.
Quoted 10 lines
Quoted 6 lines
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.
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.
Quoted 74 lines
Quoted 18 lines
Gerrit documentation for doing this type of thing can be found here:
https://gerrit-review.googlesource.com/Documentation/rest-api-changes.htmlQuoted 4 lines
A related question is: Who are the machine maintainers? Off the top of my head I feel the following machines machines match with people:
Quoted 8 lines
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>Quoted 2 lines
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.
Quoted 51 lines
Quoted 16 lines
Less obvious are the following machines:
meta-openpower/meta-ibm/meta-palmetto
meta-openpower/meta-ibm/meta-garrison
meta-openpower/meta-rackspace/meta-barreleyeYet 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.
Quoted 7 lines
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_MAINTAINERSfile 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.
Quoted 6 lines
Quoted 2 lines
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!
Quoted 6 lines
Thanks for the detailed reply, clearly it's something that's bothered people besides Joel and I. If it's bothered people beyond us and Google, I'd love to hear your solutions if you've got so far as having any.
Andrew
Cheers!