[DISCUSS] Required number of PR review approvals

[nknize]

Opening this as a community discuss issue to solicit some thoughts and feedback.

Before PR's are allowed to be merged the OpenSearch repository enforces a one approval requirement while the OpenSearch-Dashboards repository enforces two. The technical reason behind this was to test the CI load under two different requirements. The most costly CI task is ./gradlew check since it runs full unit, integration, smoke, and varying cluster tests. Depending on the level of collaboration and PR churn, we wanted to make sure we balanced the CI load to keep up with demand (of course we can always work to throw more compute power if needed).

While we gather some of this information in the early days of the project, we also wanted to solicit thoughts from the community around these kinds of requirements to avoid unilaterally making decisions. I'm curious what the community would like to see? One approval requirement? Two approval? Of course I think this will vary by repository and possibly even by feature implementation (since some features or repositories might demand more expert domain knowledge than others).

Discuss, post thoughts, don't hold back.

[dblock]

@nknize what would be the non-technical reasons/advantages to ask for more than one approval?

[bbarani]

I would recommend at the least a 2 PR review to prevent malicious code. Especially for scenarios like these. https://www.zdnet.com/article/open-source-software-how-many-bugs-are-hidden-there-on-purpose/

[nknize]

That's a good question @dblock! I'm curious what others think here and @bbarani raises one good reason. Another might be to try to discourage "buddy commits"? I'm all for low friction, but I've seen a one approval requirement resulting in back-channeling a buddy to approve your PR so it can be quickly merged

[mikemccand]

I don't think there should be a hard minimum on the number of approvals required for change!

Rather, it should be at the discretion of the committer who pushes the change. And each committer here can gracefully learn over time where that threshold/bar should be.

E.g. some PRs can and should go in, immediately, with zero approvals, such as a trivial change fixing a typo in a comment or exception message. Others are super hairy and possibly controversial and should see good discussion/iteration from many developers before pushing. Let our CI builds, or the automatic gradle check for each PR, catch any silly mistakes that cause test failures. Let's lead by example on how to fully utilize AWS resources to integrate automation to catch such honest accidents.

There are indeed horror stories, like @bbarani's example above, of malicious code sneaking into and being released from important open-source projects. Yes, it is a real risk, but it is not common, and we should not design our development rules/practices around such rare exceptional cases. Rather, design them for the common case where a developer is trying to make an honest, good, important, helpful change and we seek to absolutely minimize the friction, especially for new developers.

E.g. not requiring an iCLA is a delightful and refreshingly obvious approach to reducing friction for new developers. I was at Elastic when they decided to add the "mandatory iCLA" and fought (and clearly failed) to prevent such a nasty requirement of new community members.

Also, remember that even after a change is pushed, other developers in the community can and should still look at what was pushed and raise objections, make a follow-on PR to improve it, ask for revert, etc. It is not until the bits are set free in an an actual GA release (hmm: do we have a community process for voting on releases yet?) until possible damage from malicious changes and accidental bugs might have impact.

And we can/should seek to statically reduce that possibility, e.g. we already removed Elasticsearch's sneaky and dangerous "phone home" feature in OpenSearch, we should double down on the JVM security manager so OpenSearch runs inside a "self imposed Java jail", etc.

[nknize]

So refreshing to see this kind of healthy dialogue in the open!

do we have a community process for voting on releases yet?

Great question!!! There's a WIP discuss issue for that! that needs fixing and input from you folks that are much smarter than me!

remember that even after a change is pushed, other developers in the community can and should still look at what was pushed and raise objections, make a follow-on PR to improve it, ask for revert, etc.

SUCH an important point about OSS and transparency here! These definitely are not one-way doors and there is always room for improvement.

we should double down on the JVM security manager so OpenSearch runs inside a "self imposed Java jail", etc

Oooh!!!! We need a "help wanted" issue for this! I bet there are some contributors out there willing to make this happen!

[mikemccand]

So refreshing to see this kind of healthy dialogue in the open!

+1, these are vital ground rules for building a healthy truly open community.

we should double down on the JVM security manager so OpenSearch runs inside a "self imposed Java jail", etc

Oooh!!!! We need a "help wanted" issue for this! I bet there are some contributors out there willing to make this happen!

OK I opened #528 for this!

[rmuir]

I agree with @mikemccand, there should be one mandatory reviewer, and that's the automated one (github action, bot, whatever calling the appropriate gradle)

Java is so ripe for static analysis, but lots of projects don't even fail on warnings from javac (I haven't checked the setup here). At apache lucene, we also use the ecj compiler to do a separate pass since it has a bunch of additional checks. For style consistency we just enforce everything is formatted with gradlew tidy (uses spotless) and that's another thing not to worried about. Maybe some of this build logic can be easily poached from lucene's gradle build?

Better to make use of the human time with higher-level review.

[nknize]

there should be one mandatory reviewer, and that's the automated one

I agree with this. The more "fail early" checks that can be automated the better, and leave the overall logic flow to human review.

There's quite a bit poached from lucene's build but a lot more that I think could be leveraged. Another area that could use some help. Spotless is used here so that helps for code consistency, and there's a feature branch to bring in the javadoc enforcements that Lucene has to ensure at least the public facing APIs are documented instead of relying on a separate asciidoc or markdown "documentation" repo that is never enforced to be updated by the original author.

ecj isn't used at all in this codebase and I think that's another great suggestion that would definitely help. I don't think there's an overall "help wanted" issue for this so I may start one to capture the ideas.

[itiyamas]

I think we should define maintainer areas of responsibility as suggested here #537.

We have good documentation on how and when to create a PR. But what happens once the PR is raised is missing. e.g. after how long should I wait to follow-up for starting gradle checks or what is the criteria to start it? How is a reviewer selected for a particular module? Once a PR is approved by seasoned reviewers, how long before it is merged or at least how long before any update on the merge is expected.

[mikemccand]

we already removed Elasticsearch's sneaky and dangerous "phone home" feature in OpenSearch

Some quick followups/corrections about my statement from above:

• It is Kibana (NOT Elasticsearch) that has the "phone home" feature
• This feature is indeed enabled by default as described here: https://www.google.com/url?q=https://www.elastic.co/guide/en/kibana/master/telemetry-settings-kbn.html%23telemetry-general-settings&sa=D&source=hangouts&ust=1618598703063000&usg=AFQjCNH9NoIuADga_Vgwgjlha-3Pz4OE9A
• In OpenSearch, we have not removed this feature, but rather turned it off by default so users can freely decide whether they want to enable it, and also so the OpenSearch developer community can decide what to do with this feature going forward

More details here: https://discuss.opendistrocommunity.dev/t/preparing-opensearch-and-opensearch-dashboards-for-release/5567

Thanks @s1monw and @nknize for clarifying! High velocity code base here, yay :)

[dblock]

We've generally tried to document the review process here. What should we do with this issue?

[CEHENKLE]

Based on the comments above, I've set branches to have 1 approver required. Happy to revisit this at a later date if we want to add more or remove the constraint.

Thanks!
/C