Define when a PR is "ready to merge" #4625
Conversation
Signed-off-by: Anthony J Mirabella <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #4625 +/- ##
=======================================
Coverage 90.35% 90.35%
=======================================
Files 181 181
Lines 10591 10591
=======================================
Hits 9570 9570
Misses 797 797
Partials 224 224 Continue to review full report at Codecov.
|
| changes it should be assumed that the PR needs their review again. Other | ||
| project members (e.g. approvers, maintainers) can help with this if there are | ||
| any questions or if you forget to clear reviews. | ||
| * It has been open for review for at least one working day. This gives |
There was a problem hiding this comment.
my two cents: 24h is pretty short for substantive changes... I usually specify 48h when I'm waiting for final approval on a large/meaningful change.
|
... as for the thread that inspired this PR in #4605: in reading that exchange it's not clear whether we all even agree on the facts of the matter (namely, was this actually a significant set of changes or basically an uncontroversial refactor? was the necessary context to understand the change available when the PR was created? etc). What does seem to be clear is that the various vendors building a technical strategy around the collector want to be on equal footing as far as design decisions. Is that the thing we're trying to solve for with this (#4625) PR? |
Signed-off-by: Anthony J Mirabella <[email protected]>
There was a problem hiding this comment.
I am not going to approve this, since this behavior encourages large "unreviewable" PRs. If this gets merge as is we encourage PRs authors to send 1000+ lines PRs, see also what is proposed here #4630 which are impossible to carefully review, I would not be a maintainer of this project.
|
I'm sorry, @bogdandrutu, but I believe that you are out of step with what the other contributors and users of this project want. I do not understand how this policy would "encourage PRs authors to send 1000+ lines PRs". It says nothing about PR size, only what procedural steps must be satisfied before a PR is ready to be merged. We have used the same policy on the OTel Go repos for a long time and have not appeared to encourage overly-large PRs. @tigrannajaryan as the other maintainer on this repository will you please provide your feedback? |
Because of the time constraints and artificial requirement of companies review, also because of inactivity of a lot of the approvers, everyone will create a large PR and pray to God to get necessary approvers from different companies. Also this artificially increases the "power" of less represented companies, by the artificial requirement to have approvers from different companies. I do understand why this is important for a project like specification, but does not make any sense here. |
For the vast majority of PRs this would change nothing. Any PR that is approved by anyone not from Splunk and merged would satisfy this policy as the merging maintainer would provide the second approval. In contrib it becomes even less of an issue as there are non-Splunk maintainers.
I don't understand this perspective. Adding this requirement wouldn't increase the "power" of any company except to the extent that it would decrease the "power" of Splunk and put all participants on a level playing field. Any "less represented compan[y]" with an approver can already put a block on a PR.
It makes perfect sense here, but I believe that you are blinded to its necessity as you benefit from the status quo. |
@Aneurysm9 from your comments it is clearly that you have a problem with "Splunk" maintainers, since you refer only for Splunk people.
This is yet one more time when you have a problem with Splunk not want the best for the project, or you will argue that the best for the project is to remove Splunk :))
I may be ... I definitely benefit, especially when I look at this https://all.devstats.cncf.io/d/66/developer-activity-counts-by-companies?orgId=1&var-period_name=Last%202%20years&var-metric=contributions&var-repogroup_name=All&var-country_name=All&var-companies=All |
|
This to me sound like the real motivation is not to make the project more successful, but instead to reduce Splunk's influence and contribution to a project. |
|
I have no problem with Splunk or maintainers who represent Splunk. I have a problem with a single company having the ability to unilaterally execute changes to a community project and exercising that ability in non-transparent ways that appear to be related to their internal strategy. Splunk is the only company in that position in this repository, so of course I have mentioned Splunk and not other companies. I am in no way advocating for the removal of Splunk. I am advocating for the removal of Splunk's privileged position with respect to this project. I think that is in the best interest of the project. @open-telemetry/governance-committee I believe that we are unlikely to have further productive discussions on this matter in this forum and would ask for your intervention. |
|
Added to next GC meeting agenda |
|
Two things... Tactical question about what the GC is meant to discuss
What is the GC meant to decide/discuss here? And do we want to either include @Aneurysm9 in our Thurs meeting? (Or should @bogdandrutu recuse himself?? Otherwise it seems like an asymmetric debate since in a "normal" GC meeting Bogdan would be part of the discussion, but Anthony would not) About the larger issues here...I really do see both points of view in this thread. I trust that Splunk has no bad intentions regarding the collector, and yet I recognize that there is opacity and an imbalanced playing field for non-Splunk community members when it comes to influencing the technical strategy for the collector. We all want a trusting group of contributors here – not just because it feels good, but because groups of engineers who trust each other are frankly far more efficient. Thinking out loud: is there benefit to requiring that the Collector SIG have a multi-vendor and well-documented strategy and technical roadmap, and then allowing maintainers more latitude to move fast (even if it's a single vendor approving+merging) as long as the movement is consistent with the aforementioned strategy and technical roadmap. If changes get merged quickly (i.e., with only a single employer approving+merging) that are deemed, retroactively, to be misaligned to the strategy+roadmap, then there could be a policy to roll those changes back and discuss before rolling forward again. If it's not obvious, my goal above is to address the strategic alignment and trust problems directly, rather than creating additional per-PR friction which might – ironically – erode trust further, contentious PR by contentious PR. All of this being said, if we can't find a way to create true strategic+technical alignment for the Collector SIG, requiring multiple employers to approve changes seems like a viable check-and-balance. |
I think again if it is a matter of applying the rules then @Aneurysm9 should ask TC to decide on this PR. If this is about creating rules then GC should create org rules about how to merge PRs. If the discussion is just about this PR (and not making the rule for the entire org) then @Aneurysm9 should be included correct. Not sure what GC can discuss about an individual PR, but if that is the case definitely include @Aneurysm9. |
Let's discuss this in the TC too, as this seems a 'hot' topic. |
|
I view this as a project governance policy as it applies regardless of the technical impact of proposed changes. To that extent I think it is within the scope of the GC and the GC should resolve the issue if agreement cannot be reached within the SIG. It could certainly be argued that this relates to "[d]evelopment process and ... coding standards", which would be within the purview of the TC, but I do not expect everyone to be satisfied with any result there and thus it will ultimately come to the GC as the "final escalation path for any contested OpenTelemetry related decision". Given that the joint GC and TC have discussed this issue recently it is probably best that it be handled jointly and that a policy be established for the OpenTelemetry project as a whole and not just for this repository. |
|
(All right, I am back to work after the winter break, sorry for delayed response). [Meta] Quick reminder to everyone: in Otel we managed to create an environment of positive collaboration in the industry, often between competitors, which to me is extremely valuable. Let's keep it this way and work on improving what needs to be improved. @Aneurysm9 can you please clarify, I am not sure I understand which of the following this change attempts to solve:
I want to make sure I clearly understand your point of view and what you see as a problem (which, you are right, we as maintainers may not necessarily experience as a problem). |
My initial frustration, and the proximal cause of this PR is the latter, though I think that my concern is more with the lack of sufficient oversight and less with the fact that both maintainers of this project represent the same company. A requirement of approval from members representing multiple companies is, in some sense, a proxy for consensus within the community. It is an imperfect proxy, but to my mind better than the current situation, which seems to me to be corrosive to that environment of positive collaboration that you rightly mention is an extremely valuable attribute of this project. That said, it is my understanding that there has been a proposal circulated among the TC and GC that shares many of the same attributes of this proposal but that would also seek to reduce the friction for non-maintainer approvers seeking to merge PRs that have achieved consensus. I would be happy to have that proposal supplant this one, but doing nothing is not an option from my perspective.
I have tried to lay out my concerns in a way that refrains from directing blame or culpability at any one person, and I don't think that any one person is entirely at fault, but perhaps I have not been clear enough and so I will stop speaking around what I see as the heart of the matter. It appears to me that Bogdan views the collector as his fiefdom to do with as he will without input from, or accountability to, anyone. He seems to make changes with little or no description of their intent or rationale, has them reviewed by someone from Splunk, and merges them before anyone can notice that a change has been proposed. He appears to hold others to a higher standard than he holds himself. I do not feel that he has the judgment or temperament to be the de facto sole maintainer of this project. As a community-run project we cannot afford to allow any individual to act as if they were BDFL. The policy I have proposed here is one of a series of steps that I believe need to be taken to ensure that the community of collector developers and users can participate in this project on an equal footing and without fear or favor. I also think that the collector and collector-contrib repos need more approvers and maintainers (perhaps as much as four times the current roster) and that effort needs to be put in by the current maintainers to develop that pipeline. I don't believe that any one company should be represented by more than 49% of maintainers of a repository that has more that one maintainer, and I don't think any repository should have a sole maintainer, though that may be a challenge that is not amenable to policy-based solutions in some cases. |
|
I agree with @bhs's point about a trusting group - the topic of requiring reviews from multiple employers came up in the past in opentelemetry-java-instrumentation but we decided we don't need it, we trust all the maintainers, even if two Splunk maintainers merge in a PR. In practice, we notice this rarely happens for core concepts and more for instrumentation libraries - everyone seems to be on the same page. I don't believe that requiring two employers to merge can actually help with the issue of visibility to community members - it's easy for two members from different employers with a strong trust relationship to merge each others PRs quickly in the same way as it is for same employers, there just doesn't seem to be anything specific to the aspect of employer here. Policies, which apply to everyone including maintainers, for better review before code is written could perhaps help. For example, requiring simple design docs for changes affecting core APIs in a significant way or discussion on an issue, could help. I think the key point is that not every PR should have to be slowed down, for example with a one-two day buffer for reviews, but that initial time spent could be spent before a large effort to get a diverse set of community members on the same page in a worthwhile way. FWIW, I don't know the details here and whether this happened or not, but based on the discussions assuming it didn't. Another issue may have been too much discussion of collector direction in internal company forums / meetings. I think as maintainers in OTel, it is our responsibility to ensure this doesn't happen too much, at least in a way that can make other members feel affected. I try my best for AWS, and myself strongly avoid Quip for discussions about upstream topics. But it's hard and I don't feel so successful at this, my team still writes lots of Quip docs where I wish the discussion was more open. I can understand the tendency to have internal discussions within a company since people are used to it, but hopefully we can at least strive to reduce that where possible. Note that admittedly, this would be helped a bit with a 2-employer merge policy, but I don't feel this to be the brunt of the issue vs just trusting members having a tendency to merge fast. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Signed-off-by: Anthony J Mirabella [email protected]
Description: Copied definition of "ready to merge" from OTel Go.