[nova][all][ptg] Summary: Same-Company Approvals
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.)
Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
Summary: - There is a (currently unwritten? at least for Nova) rule that a patch should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to: - Usually (but not always) relevant when the patch was proposed by member of same company - N/A for trivial things like typo fixes - The issue is: - Should the rule be abolished? and/or - Should the rule be written down?
Consensus (not unanimous): - The rule should not be abolished. There are cases where both the impetus and the subject matter expertise for a patch all reside within one company. In such cases, at least one core from another company should still be engaged and provide a "procedural +2" - much like cores proxy SME +1s when there's no core with deep expertise. - If there is reasonable justification for bending the rules (e.g. typo fixes as noted above, some piece of work clearly not related to the company's interest, unwedging the gate, etc.) said justification should be clearly documented in review commentary. - The rule should not be documented (this email notwithstanding). This would either encourage loopholing or turn into a huge detailed legal tome that nobody will read. It would also *require* enforcement, which is difficult and awkward. Overall, we should be able to trust cores to act in good faith and in the appropriate spirit.
efried .
On Sat, May 4, 2019, 16:48 Eric Fried openstack@fried.cc wrote:
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.)
Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
Summary:
- There is a (currently unwritten? at least for Nova) rule that a patch
should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to:
- Usually (but not always) relevant when the patch was proposed by
member of same company
- N/A for trivial things like typo fixes
- The issue is:
- Should the rule be abolished? and/or
- Should the rule be written down?
Consensus (not unanimous):
- The rule should not be abolished. There are cases where both the
impetus and the subject matter expertise for a patch all reside within one company. In such cases, at least one core from another company should still be engaged and provide a "procedural +2" - much like cores proxy SME +1s when there's no core with deep expertise.
- If there is reasonable justification for bending the rules (e.g. typo
fixes as noted above, some piece of work clearly not related to the company's interest, unwedging the gate, etc.) said justification should be clearly documented in review commentary.
- The rule should not be documented (this email notwithstanding). This
would either encourage loopholing or turn into a huge detailed legal tome that nobody will read. It would also *require* enforcement, which is difficult and awkward. Overall, we should be able to trust cores to act in good faith and in the appropriate spirit.
efried .
Keystone used to have the same policy outlined in this email (with much of the same nuance and exceptions). Without going into crazy details (as the contributor and core numbers went down), we opted to really lean on "Overall, we should be able to trust cores to act in good faith". We abolished the rule and the cores always ask for outside input when the familiarity lies outside of the team. We often also pull in cores more familiar with the code sometimes ending up with 3x+2s before we workflow the patch.
Personally <non-core/non-leadership hat> I don't like the "this is an unwritten rule and it shouldn't be documented"; if documenting and enforcement of the rule elicits worry of gaming the system or being a dense some not read, in my mind (and experience) the rule may not be worth having. I voice my opinion with the caveat that every team is different. If the rule works, and helps the team (Nova in this case) feel more confident in the management of code, the rule has a place to live on. What works for one team doesn't always work for another.
Morgan Fainberg morgan.fainberg@gmail.com wrote:
On Sat, May 4, 2019, 16:48 Eric Fried openstack@fried.cc wrote:
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.)
Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
I didn't pipe up during the PTG discussion because a) I missed the first 5-10 minutes and hence probably some important context, and b) I've not been a nova contributor long enough to be well-informed on this topic. Apologies if that was the wrong decision.
But I do have a few thoughts on this, which I'll share below. Given b), take them with a pinch of salt ;-)
Firstly, I was impressed with the way this topic was raised and discussed, and I think that is a very encouraging indicator for the current health of nova contributor culture. We're in a good place :-)
Summary:
- There is a (currently unwritten? at least for Nova) rule that a patch
should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to:
- Usually (but not always) relevant when the patch was proposed by
member of same company
- N/A for trivial things like typo fixes
- The issue is:
- Should the rule be abolished? and/or
- Should the rule be written down?
Consensus (not unanimous):
[snipped]
Keystone used to have the same policy outlined in this email (with much of the same nuance and exceptions). Without going into crazy details (as the contributor and core numbers went down), we opted to really lean on "Overall, we should be able to trust cores to act in good faith". We abolished the rule and the cores always ask for outside input when the familiarity lies outside of the team. We often also pull in cores more familiar with the code sometimes ending up with 3x+2s before we workflow the patch.
Personally <non-core/non-leadership hat> I don't like the "this is an unwritten rule and it shouldn't be documented"; if documenting and enforcement of the rule elicits worry of gaming the system or being a dense some not read, in my mind (and experience) the rule may not be worth having. I voice my opinion with the caveat that every team is different. If the rule works, and helps the team (Nova in this case) feel more confident in the management of code, the rule has a place to live on. What works for one team doesn't always work for another.
+1 - I'm not wildly enthusiastic about the "keep it undocumented" approach either.
Here's my stab at handling some of the objections to a written policy.
- The rule should not be documented (this email notwithstanding). This
would either encourage loopholing
I don't see why the presence of a written rule would encourage people to deliberately subvert upstream trust any more than they might otherwise do.
And a rule with loopholes is still a better deterrent than no rule at all. This is somewhat true for deliberate subversions of trust (which I expect are non-existent or at least extremely rare), but especially true for accidental subversions of trust which could otherwise happen quite easily due to not fully understanding how upstream works.
or turn into a huge detailed legal tome that nobody will read.
I don't think it has to. It's not a legal document, so there's no need to attempt to make it like one. If there are loopholes which can't easily be covered by a simple rewording, then so be it. If the policy only catches 50% of cases, it's still helping. So IMHO the existence of loopholes doesn't justify throwing the baby out with the bathwater.
It would also *require* enforcement, which is difficult and awkward. Overall, we should be able to trust cores to act in good faith and in the appropriate spirit.
I agree that enforcement would be difficult and awkward, and that we should be able to trust cores. But in the unlikely and unfortunate situation that a problem arose in this space, the lack of a written policy wouldn't magically solve that problem. in fact it would make it even *harder* to deal with, because there'd be nothing to point to in order to help explain to the offender what they were doing wrong. That would automatically make any judgement appear more subjective than objective, and therefore more prone to being taken personally.
All,
Cinder has been working with the same unwritten rules for quite some time as well with minimal issues.
I think the concerns about not having it documented are warranted. We have had question about it in the past with no documentation to point to. It is more or less lore that has been passed down over the releases. :-)
At a minimum, having this e-mail thread is helpful. If, however, we decide to document it I think we should have it consistent across the teams that use the rule. I would be happy to help draft/review any such documentation.
Jay
On 5/4/2019 8:19 PM, Morgan Fainberg wrote:
On Sat, May 4, 2019, 16:48 Eric Fried openstack@fried.cc wrote:
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.) Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance Summary: - There is a (currently unwritten? at least for Nova) rule that a patch should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to: - Usually (but not always) relevant when the patch was proposed by member of same company - N/A for trivial things like typo fixes - The issue is: - Should the rule be abolished? and/or - Should the rule be written down? Consensus (not unanimous): - The rule should not be abolished. There are cases where both the impetus and the subject matter expertise for a patch all reside within one company. In such cases, at least one core from another company should still be engaged and provide a "procedural +2" - much like cores proxy SME +1s when there's no core with deep expertise. - If there is reasonable justification for bending the rules (e.g. typo fixes as noted above, some piece of work clearly not related to the company's interest, unwedging the gate, etc.) said justification should be clearly documented in review commentary. - The rule should not be documented (this email notwithstanding). This would either encourage loopholing or turn into a huge detailed legal tome that nobody will read. It would also *require* enforcement, which is difficult and awkward. Overall, we should be able to trust cores to act in good faith and in the appropriate spirit. efried .Keystone used to have the same policy outlined in this email (with much of the same nuance and exceptions). Without going into crazy details (as the contributor and core numbers went down), we opted to really lean on "Overall, we should be able to trust cores to act in good faith". We abolished the rule and the cores always ask for outside input when the familiarity lies outside of the team. We often also pull in cores more familiar with the code sometimes ending up with 3x+2s before we workflow the patch.
Personally <non-core/non-leadership hat> I don't like the "this is an unwritten rule and it shouldn't be documented"; if documenting and enforcement of the rule elicits worry of gaming the system or being a dense some not read, in my mind (and experience) the rule may not be worth having. I voice my opinion with the caveat that every team is different. If the rule works, and helps the team (Nova in this case) feel more confident in the management of code, the rule has a place to live on. What works for one team doesn't always work for another.
On 2019-05-07 15:06:10 -0500 (-0500), Jay Bryant wrote:
Cinder has been working with the same unwritten rules for quite some time as well with minimal issues.
I think the concerns about not having it documented are warranted. We have had question about it in the past with no documentation to point to. It is more or less lore that has been passed down over the releases. :-)
At a minimum, having this e-mail thread is helpful. If, however, we decide to document it I think we should have it consistent across the teams that use the rule. I would be happy to help draft/review any such documentation.
[...]
I have a feeling that a big part of why it's gone undocumented for so long is that putting it in writing risks explicitly sending the message that we don't trust our contributors to act in the best interests of the project even if those are not aligned with the interests of their employer/sponsor. I think many of us attempt to avoid having all activity on a given patch come from people with the same funding affiliation so as to avoid giving the impression that any one organization is able to ram changes through with no oversight, but more because of the outward appearance than because we don't trust ourselves or our colleagues.
Documenting our culture is a good thing, but embodying that documentation with this sort of nuance can be challenging.
Jeremy Stanley fungi@yuggoth.org wrote:
On 2019-05-07 15:06:10 -0500 (-0500), Jay Bryant wrote:
Cinder has been working with the same unwritten rules for quite some time as well with minimal issues.
I think the concerns about not having it documented are warranted. We have had question about it in the past with no documentation to point to. It is more or less lore that has been passed down over the releases. :-)
At a minimum, having this e-mail thread is helpful. If, however, we decide to document it I think we should have it consistent across the teams that use the rule. I would be happy to help draft/review any such documentation.
[...]
I have a feeling that a big part of why it's gone undocumented for so long is that putting it in writing risks explicitly sending the message that we don't trust our contributors to act in the best interests of the project even if those are not aligned with the interests of their employer/sponsor. I think many of us attempt to avoid having all activity on a given patch come from people with the same funding affiliation so as to avoid giving the impression that any one organization is able to ram changes through with no oversight, but more because of the outward appearance than because we don't trust ourselves or our colleagues.
Documenting our culture is a good thing, but embodying that documentation with this sort of nuance can be challenging.
That's a good point. Maybe that risk could be countered by explicitly stating something like "this is not currently an issue within the community, and it has rarely, if ever, been one in the past; therefore this policy is a preemptive safeguard rather than a reactive one" ?
On May 8, 2019, at 10:45 AM, Adam Spiers aspiers@suse.com wrote:
I have a feeling that a big part of why it's gone undocumented for so long is that putting it in writing risks explicitly sending the message that we don't trust our contributors to act in the best interests of the project even if those are not aligned with the interests of their employer/sponsor. I think many of us attempt to avoid having all activity on a given patch come from people with the same funding affiliation so as to avoid giving the impression that any one organization is able to ram changes through with no oversight, but more because of the outward appearance than because we don't trust ourselves or our colleagues. Documenting our culture is a good thing, but embodying that documentation with this sort of nuance can be challenging.
That's a good point. Maybe that risk could be countered by explicitly stating something like "this is not currently an issue within the community, and it has rarely, if ever, been one in the past; therefore this policy is a preemptive safeguard rather than a reactive one" ?
I think that’s a good approach. This way if such a situation comes up and people wonder why others are questioning it, it will be all above-board. The downside of *not* documenting this concern is that in the future if it is ever needed to be mentioned, the people involved might feel that the community is suddenly ganging up against their company, instead of simply following documented policy.
-- Ed Leafe
On Wed, May 8, 2019, 09:09 Ed Leafe ed@leafe.com wrote:
On May 8, 2019, at 10:45 AM, Adam Spiers aspiers@suse.com wrote:
I have a feeling that a big part of why it's gone undocumented for so
long is that putting it in writing risks explicitly sending the message that we don't trust our contributors to act in the best interests of the project even if those are not aligned with the interests of their employer/sponsor. I think many of us attempt to avoid having all activity on a given patch come from people with the same funding affiliation so as to avoid giving the impression that any one organization is able to ram changes through with no oversight, but more because of the outward appearance than because we don't trust ourselves or our colleagues.
Documenting our culture is a good thing, but embodying that
documentation with this sort of nuance can be challenging.
That's a good point. Maybe that risk could be countered by explicitly
stating something like "this is not currently an issue within the community, and it has rarely, if ever, been one in the past; therefore this policy is a preemptive safeguard rather than a reactive one" ?
I think that’s a good approach. This way if such a situation comes up and people wonder why others are questioning it, it will be all above-board. The downside of *not* documenting this concern is that in the future if it is ever needed to be mentioned, the people involved might feel that the community is suddenly ganging up against their company, instead of simply following documented policy.
-- Ed Leafe
In general I would rather see trust be pushed forward. The cores are explicitly trusted individuals within a team. I would encourage setting this policy aside and revisit if it ever becomes an issue. I think this policy, preemptive or not, highlights a lack of trust. It is another reason why Keystone team abolished the rule. AI.kuch prefer trusting the cores with landing code with or without external/additional input as they feel is appropriate.
There are remedies if something lands inappropriately (revert, removal of core status, etc). As stated earlier in the quoted email, this has never or almost never been an issue.
With that said, I don't have a strongly vested interest outside of seeing our community succeeding and being as open/inclusive as possible (since most contributions I am working on are not subject to this policy). As long as the policy isn't strictly tribal knowledge, we are headed in the right direction.
Morgan Fainberg morgan.fainberg@gmail.com wrote:
In general I would rather see trust be pushed forward. The cores are explicitly trusted individuals within a team. I would encourage setting this policy aside and revisit if it ever becomes an issue. I think this policy, preemptive or not, highlights a lack of trust.
IMHO it wouldn't highlight a lack of trust if it explicitly said that there is no current problem in the community.
But it's not just about trust. There's also the issue of simple honest lack of awareness, even by diligent newbie cores with the very finest of intentions.
Honestly, if I hadn't stumbled across this conversation at the PTG, and later became core on a project, it might have never crossed my mind that it might be better in some scenarios to avoid giving W+1 on a review where +2 was only given by colleagues at my company. Indeed, the fact that we currently (and hopefully indefinitely) enjoy the ability to trust the best interests of others cores would probably make me *more* susceptible to accidentally introducing company-oriented bias without realising it.
In contrast, if there was an on-boarding document for new cores which raised awareness of this, I would read that when becoming a core, and then vet myself for employer-oriented bias before every +2 and W+1 I subsequently gave.
It is another reason why Keystone team abolished the rule. AI.kuch prefer trusting the cores with landing code with or without external/additional input as they feel is appropriate.
There are remedies if something lands inappropriately (revert, removal of core status, etc). As stated earlier in the quoted email, this has never or almost never been an issue.
With that said, I don't have a strongly vested interest outside of seeing our community succeeding and being as open/inclusive as possible (since most contributions I am working on are not subject to this policy). As long as the policy isn't strictly tribal knowledge, we are headed in the right direction.
Agreed. Any suggestions on how to prevent it being tribal? The only way I can think of is documenting it, but maybe I'm missing a trick.
On Wed, May 8, 2019 at 11:27 AM Adam Spiers aspiers@suse.com wrote:
Morgan Fainberg morgan.fainberg@gmail.com wrote:
In general I would rather see trust be pushed forward. The cores are explicitly trusted individuals within a team. I would encourage setting this policy aside and revisit if it ever becomes an issue. I think this policy, preemptive or not, highlights a lack of trust.
IMHO it wouldn't highlight a lack of trust if it explicitly said that there is no current problem in the community.
But it's not just about trust. There's also the issue of simple honest lack of awareness, even by diligent newbie cores with the very finest of intentions.
Honestly, if I hadn't stumbled across this conversation at the PTG, and later became core on a project, it might have never crossed my mind that it might be better in some scenarios to avoid giving W+1 on a review where +2 was only given by colleagues at my company. Indeed, the fact that we currently (and hopefully indefinitely) enjoy the ability to trust the best interests of others cores would probably make me *more* susceptible to accidentally introducing company-oriented bias without realising it.
In contrast, if there was an on-boarding document for new cores which raised awareness of this, I would read that when becoming a core, and then vet myself for employer-oriented bias before every +2 and W+1 I subsequently gave.
It is another reason why Keystone team abolished the rule. AI.kuch prefer trusting the cores with landing code with or without external/additional input as they feel
is
appropriate.
There are remedies if something lands inappropriately (revert, removal of core status, etc). As stated earlier in the quoted email, this has never
or
almost never been an issue.
With that said, I don't have a strongly vested interest outside of seeing our community succeeding and being as open/inclusive as possible (since most contributions I am working on are not subject to this policy). As
long
as the policy isn't strictly tribal knowledge, we are headed in the right direction.
Agreed. Any suggestions on how to prevent it being tribal? The only way I can think of is documenting it, but maybe I'm missing a trick.
Unfortunately, in this case it's "tribal" or "documented". No "one weird trick" here as far as I know ;). --Morgan
On 5/8/19 1:50 PM, Morgan Fainberg wrote:
On Wed, May 8, 2019 at 11:27 AM Adam Spiers <aspiers@suse.com mailto:aspiers@suse.com> wrote:
Morgan Fainberg <morgan.fainberg@gmail.com <mailto:morgan.fainberg@gmail.com>> wrote: >In general I would rather see trust be pushed forward. The cores are >explicitly trusted individuals within a team. I would encourage setting >this policy aside and revisit if it ever becomes an issue. I think this >policy, preemptive or not, highlights a lack of trust. IMHO it wouldn't highlight a lack of trust if it explicitly said that there is no current problem in the community. But it's not just about trust. There's also the issue of simple honest lack of awareness, even by diligent newbie cores with the very finest of intentions. Honestly, if I hadn't stumbled across this conversation at the PTG, and later became core on a project, it might have never crossed my mind that it might be better in some scenarios to avoid giving W+1 on a review where +2 was only given by colleagues at my company. Indeed, the fact that we currently (and hopefully indefinitely) enjoy the ability to trust the best interests of others cores would probably make me *more* susceptible to accidentally introducing company-oriented bias without realising it. In contrast, if there was an on-boarding document for new cores which raised awareness of this, I would read that when becoming a core, and then vet myself for employer-oriented bias before every +2 and W+1 I subsequently gave. >It is another reason >why Keystone team abolished the rule. AI.kuch prefer trusting the cores >with landing code with or without external/additional input as they feel is >appropriate. > >There are remedies if something lands inappropriately (revert, removal of >core status, etc). As stated earlier in the quoted email, this has never or >almost never been an issue. > >With that said, I don't have a strongly vested interest outside of seeing >our community succeeding and being as open/inclusive as possible (since >most contributions I am working on are not subject to this policy). As long >as the policy isn't strictly tribal knowledge, we are headed in the right >direction. Agreed. Any suggestions on how to prevent it being tribal? The only way I can think of is documenting it, but maybe I'm missing a trick.Unfortunately, in this case it's "tribal" or "documented". No "one weird trick" here as far as I know ;).
Two cores, one company. You won't believe what happens next!
/me goes back to daydreaming about working on a project with enough contributors for this to be a problem :-)
On 5/8/2019 10:45 AM, Adam Spiers wrote:
Jeremy Stanley fungi@yuggoth.org wrote:
On 2019-05-07 15:06:10 -0500 (-0500), Jay Bryant wrote:
Cinder has been working with the same unwritten rules for quite some time as well with minimal issues. I think the concerns about not having it documented are warranted. We have had question about it in the past with no documentation to point to. It is more or less lore that has been passed down over the releases. :-) At a minimum, having this e-mail thread is helpful. If, however, we decide to document it I think we should have it consistent across the teams that use the rule. I would be happy to help draft/review any such documentation.
[...]
I have a feeling that a big part of why it's gone undocumented for so long is that putting it in writing risks explicitly sending the message that we don't trust our contributors to act in the best interests of the project even if those are not aligned with the interests of their employer/sponsor. I think many of us attempt to avoid having all activity on a given patch come from people with the same funding affiliation so as to avoid giving the impression that any one organization is able to ram changes through with no oversight, but more because of the outward appearance than because we don't trust ourselves or our colleagues. Documenting our culture is a good thing, but embodying that documentation with this sort of nuance can be challenging.
That's a good point. Maybe that risk could be countered by explicitly stating something like "this is not currently an issue within the community, and it has rarely, if ever, been one in the past; therefore this policy is a preemptive safeguard rather than a reactive one" ?
This thread sparked discussion in the Cinder meeting this week and I will just share that here for completeness.
Some of our newer members were not aware of this unwritten rule and it has been 'tribal' knowledge for the Cinder team for quite some time. It also has not been an issue for many years.
The long story short was: 'If it is a new feature it should be reviewed by at least one person from a different company than the author. For bugs, cores should use their best judgement.'
As to whether or not to document it, it looks from the mailing list thread like some teams have documented it and some haven't. So it would seem that letting what each team sees as most fit for their project may be the best answer.
Jay
On Tue, May 7, 2019 at 1:08 PM Jay Bryant jungleboyj@gmail.com wrote:
All,
Cinder has been working with the same unwritten rules for quite some time as well with minimal issues.
I think the concerns about not having it documented are warranted. We have had question about it in the past with no documentation to point to. It is more or less lore that has been passed down over the releases. :-)
At a minimum, having this e-mail thread is helpful. If, however, we decide to document it I think we should have it consistent across the teams that use the rule. I would be happy to help draft/review any such documentation.
Chiming in to say the manila community adopted a review policy during Stein release - most of the review policy was what we followed prior, without explicitly writing them down: https://docs.openstack.org/manila/latest/contributor/manila-review-policy.ht.... Here's a snip/snap from that policy that is relevant to this discussion:
<quote> Previously, the manila core team informally enforced a code review convention that each code change be reviewed and merged by reviewers of different affiliations. This was followed because the OpenStack Technical Committee used the diversity of affiliation of the core reviewer team as a metric for maturity of the project. However, since the Rocky release cycle, the TC has changed its view on the subject 3 4. We believe this is a step in the right direction.
While there is no strict requirement that two core reviewers accepting a code change have different affiliations. Other things being equal, we will continue to informally encourage organizational diversity by having core reviewers from different organizations. Core reviewers have the professional responsibility of avoiding conflicts of interest. </unquote>
Jay
On 5/4/2019 8:19 PM, Morgan Fainberg wrote:
On Sat, May 4, 2019, 16:48 Eric Fried openstack@fried.cc wrote:
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.)
Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
Summary:
- There is a (currently unwritten? at least for Nova) rule that a patch
should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to:
- Usually (but not always) relevant when the patch was proposed by
member of same company
- N/A for trivial things like typo fixes
- The issue is:
- Should the rule be abolished? and/or
- Should the rule be written down?
Consensus (not unanimous):
- The rule should not be abolished. There are cases where both the
impetus and the subject matter expertise for a patch all reside within one company. In such cases, at least one core from another company should still be engaged and provide a "procedural +2" - much like cores proxy SME +1s when there's no core with deep expertise.
- If there is reasonable justification for bending the rules (e.g. typo
fixes as noted above, some piece of work clearly not related to the company's interest, unwedging the gate, etc.) said justification should be clearly documented in review commentary.
- The rule should not be documented (this email notwithstanding). This
would either encourage loopholing or turn into a huge detailed legal tome that nobody will read. It would also *require* enforcement, which is difficult and awkward. Overall, we should be able to trust cores to act in good faith and in the appropriate spirit.
efried .
Keystone used to have the same policy outlined in this email (with much of the same nuance and exceptions). Without going into crazy details (as the contributor and core numbers went down), we opted to really lean on "Overall, we should be able to trust cores to act in good faith". We abolished the rule and the cores always ask for outside input when the familiarity lies outside of the team. We often also pull in cores more familiar with the code sometimes ending up with 3x+2s before we workflow the patch.
Personally <non-core/non-leadership hat> I don't like the "this is an unwritten rule and it shouldn't be documented"; if documenting and enforcement of the rule elicits worry of gaming the system or being a dense some not read, in my mind (and experience) the rule may not be worth having. I voice my opinion with the caveat that every team is different. If the rule works, and helps the team (Nova in this case) feel more confident in the management of code, the rule has a place to live on. What works for one team doesn't always work for another.
On Sat, May 04, 2019 at 07:19:48PM -0600, Morgan Fainberg wrote:
On Sat, May 4, 2019, 16:48 Eric Fried openstack@fried.cc wrote:
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.)
Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
[Thanks for the summary, Eric; I couldn't be at that session due to a conflict.]
Summary:
- There is a (currently unwritten? at least for Nova) rule that a patch
should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to:
- Usually (but not always) relevant when the patch was proposed by
member of same company
- N/A for trivial things like typo fixes
- The issue is:
- Should the rule be abolished? and/or
- Should the rule be written down?
[...]
we opted to really lean on "Overall, we should be able to trust cores to act in good faith".
Indeed. IME, this is what other mature open source projects do (e.g. Linux and QEMU, which are in the "critical path" to Nova and OpenStack).
FWIW, over the past six years, I've seen plenty of cases on 'qemu-devel' (the upstream development list fo the QEMU project) and on KVM list, where a (non-trivial) patch contribution from company-X is merged by maintainers from company-X. Of course, there is the implicit trust in that the contributor is acting in upstream's best interests first. (If not, course-correct and educate.)
- - -
This Nova "rule" (which, as Eric succintly put it, is "rife with nuance") doesn't affect me much, if at all. But allow me share my stance:
I'm of course all for diverse set of opinions and reviews from different companies as much as posisble, which I consider super healthy. So long as there are no overly iron-clad "rules" that are "unbendable".
What _should_ raise a red flag is a _pattern_. E.g. Developer-A from the company Pied Piper uploads a complex change, within a couple of days (or worse, even shorter), two more upstream "core" reivewers from Pied Piper, who are in the know about the change, pile on it and approve without giving sufficient time for other community reviewers to catch-up. (Because: "hey, we need to get Pied Piper's 'priority feature' into the current release, to get that one-up against the competitor!") *That* kind of behaviour should be called out and rebuked.
_However_.
If:
- a particular (non-trivial) change is publicly advertized well-enough (2 weeks or more), for the community developers to catch up;
- all necessary details, context and use cases are described clearly in the open, without any assumptions;
- if you've checked with other non-Pied Piper "cores" if they have any strong opinions, and gave them the time to chime in;
- if the patch receives negative comments, address it without hand-waving, explaining in _every_ detail that isn't clear to non-Pied Piper reviewers, so that in the end they can come to a clear conclusion whether it's right or not;
- you are working in the best interests of upstream, _even if_ it goes against your downstream's interests; i.e. you're sincere and sensible when operating with your "upstream hat".
Then, "core" reviewers from Pied Piper _should_ be able to merge a contribution from Pied Piper (or someone else), without a nauseating feeling of "you're just one 'wrong sneeze' away from being implicated of 'trust erosion'!" or any artificial "procedural blockers".
Of course, this requires a "heightend sense of awareness", and doing that delicate tango of balancing "upstream" vs. "downstream" hats. And I'd like to imagine contributors and maintainers are constantly striving towards it.
[...]
On Sat, May 4, 2019 at 6:48 PM Eric Fried openstack@fried.cc wrote:
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.)
Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
Summary:
- There is a (currently unwritten? at least for Nova) rule that a patch
should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to:
- Usually (but not always) relevant when the patch was proposed by
member of same company
- N/A for trivial things like typo fixes
- The issue is:
- Should the rule be abolished? and/or
- Should the rule be written down?
Consensus (not unanimous):
- The rule should not be abolished. There are cases where both the
impetus and the subject matter expertise for a patch all reside within one company. In such cases, at least one core from another company should still be engaged and provide a "procedural +2" - much like cores proxy SME +1s when there's no core with deep expertise.
- If there is reasonable justification for bending the rules (e.g. typo
fixes as noted above, some piece of work clearly not related to the company's interest, unwedging the gate, etc.) said justification should be clearly documented in review commentary.
- The rule should not be documented (this email notwithstanding). This
would either encourage loopholing or turn into a huge detailed legal tome that nobody will read. It would also *require* enforcement, which is difficult and awkward. Overall, we should be able to trust cores to act in good faith and in the appropriate spirit.
efried .
In ironic-land, we documented this [1] many moons ago. Whether that is considered a rule or a guideline, I don't know, but we haven't been sued yet and I don't recall any heated arguments/incidents about it. :)
--ruby
[1] https://wiki.openstack.org/wiki/Ironic/CoreTeam#Other_notes
Top posting to change the topic slightly: same-company approvals for the stable branch.
I brought this up at today's Nova meeting, and my takeaway was as follows. Please correct me if I got something wrong.
1. Judgment! Yes, this is hard to define. 2. If you feel uneasy about merging a thing, don't do it. 3. A backport by a stable-maint core counts as a proxy +2 if the backport is an obvious bugfix and is clean. 4. We still need 2 +2's on a patch, proxy +2's count towards that. 5. As on master, we should strive to avoid 100% same-company patches. 6. The original patch author counts. A patch from company A can be approved by 2 cores from company B. One of those cores can be the backporter, with the caveats in point 3. 7. All of this can be superseded by point 1 if necessary.
On Fri, May 10, 2019 at 1:55 PM Ruby Loo opensrloo@gmail.com wrote:
On Sat, May 4, 2019 at 6:48 PM Eric Fried openstack@fried.cc wrote:
(NB: I tagged [all] because it would be interesting to know where other teams stand on this issue.)
Etherpad: https://etherpad.openstack.org/p/nova-ptg-train-governance
Summary:
- There is a (currently unwritten? at least for Nova) rule that a patch
should not be approved exclusively by cores from the same company. This is rife with nuance, including but not limited to:
- Usually (but not always) relevant when the patch was proposed by
member of same company
- N/A for trivial things like typo fixes
- The issue is:
- Should the rule be abolished? and/or
- Should the rule be written down?
Consensus (not unanimous):
- The rule should not be abolished. There are cases where both the
impetus and the subject matter expertise for a patch all reside within one company. In such cases, at least one core from another company should still be engaged and provide a "procedural +2" - much like cores proxy SME +1s when there's no core with deep expertise.
- If there is reasonable justification for bending the rules (e.g. typo
fixes as noted above, some piece of work clearly not related to the company's interest, unwedging the gate, etc.) said justification should be clearly documented in review commentary.
- The rule should not be documented (this email notwithstanding). This
would either encourage loopholing or turn into a huge detailed legal tome that nobody will read. It would also *require* enforcement, which is difficult and awkward. Overall, we should be able to trust cores to act in good faith and in the appropriate spirit.
efried .
In ironic-land, we documented this [1] many moons ago. Whether that is considered a rule or a guideline, I don't know, but we haven't been sued yet and I don't recall any heated arguments/incidents about it. :)
--ruby
[1] https://wiki.openstack.org/wiki/Ironic/CoreTeam#Other_notes
participants (11)
-
Adam Spiers
-
Artom Lifshitz
-
Ben Nemec
-
Ed Leafe
-
Eric Fried
-
Goutham Pacha Ravi
-
Jay Bryant
-
Jeremy Stanley
-
Kashyap Chamarthy
-
Morgan Fainberg
-
Ruby Loo