Add PR review policies

[zanetworker]

What would you like to be added: In our PR checklist we don't yet have a review policy, we should add one that includes:

• PR review response time and Request Changes enforcement.
• ....

[stoyanr]

Meeting Minutes 10.06.2020:

Initial agenda:

• How could we make Gardener code reviews happen faster and distribute them better.
• Would it make sense to introduce Gardener-specific coding guidelines, what these may contain, and how could we create them (not discussed).
• How could we ensure that when a reviewer requests changes he follows up, as otherwise this leads to big delays (proposed by Adel).

Summary of outcomes:

• Higher effort to rebase / keep-up that negatively affects developer productivity should be balanced with better quality of the code when more people are reviewing.
• "Primary" reviewers should review within 2 weeks, while "optional" can review. Primary reviewers are defined by GitHub teams.
• We should try to keep PRs small enough to make reviews easier / less intimidating. We should raise the awareness about this, however we were not able to identify an objective measure for PR size.
• When introducing a feature, we should avoid refactoring existing code, to keep the PR smaller and self-contained (and vice versa).
• PR author should /assign the right person / people to the PR and actively drive the PR reviews so that the process completes as fast as possible.
• The bot could assign a concrete team based on /area labels (?, proposed but no final agreement).
• For many PRs, we need an opinion from a "domain expert" in addition to an approval from an "approver" (?, no final agreement how to achieve this).
• We could make reviews part of daily syncs to organise ourselves who should review what (?, proposed but most participants sceptical).
• Everybody should go through PRs at least once per day and invest at least 30 minutes (preferably more) in reviews. However, we were so far not able to identify ways to raise the likelihood of this happening.
• We should follow consistently a promotion process from "reviewer" to "approver", similar to the one in the Kubernetes community, based on the demonstrated experience of reviewers, number of reviews, etc. So far such a process is not formalised.
• We should raise the awareness that if a reviewer requests changes he or she should also follow-up within max 2 days of the requested changes being introduced. After 2 days of non-action, such reviews should be dismissed to enable merging. The author is still responsible to make sure that the reviewer is aware about this changes.
• Efforts to make integration testing easier should be ramped up as this has the potential to reduce the effort to manually test the PR by the reviewer.

[g-pavlov]

We could make reviews part of daily syncs to organize ourselves who should review what (?, proposed but most participants sceptical).

The proposal was slightly different and perhaps not well understood - to regularly (once a week for example) review the status of the PRs in review and to surface and address potential issues with compliance with the PR review process that we define (not the PRs topics themselves).

In essence, it's about filtering PRs that have no reviewer or have been in limbo for too long after review was requested. The event can be used then to assign and re-assign PRs for reviews to keep their pulse beating and improve the load distribution and make it more transparent. I've observed a similar practice in k8s SIGs and it takes about 10-15 minutes on their bi-weekly meetings.

All things considered, it's a zero-investment practice that we can employ today and it can help until we come up with tool-assisted approaches. Such practice will build up the culture of consciousnesses, without which no tools or process will work.

Technically:

• The exact interval for such reviews would depend on what we define as PR review "sla" and how are we doing with it: once a week, bi-weekly, monthly. It doesn't have to be daily for sure and will get occasional over the time, once the right culture is in place.
• The tool support may obsolete the need for this, introducing automatic and/or assisted review assignments and nudging and re-assignments after a PR timeframe for review expires. But I see the tool support working to sustain the community culture that has already to be there in the first place.

[g-pavlov]

To be clear - this issue is in the documentation repo because at the end we are going to formalize the decisions into a guide in the section that describes our processes. But the discussion is about the whole Gardener project, not the documentation.

[zanetworker]

@stoyanr thanks for following up. I guess now we should try to find a place to put it into a review guide.

@g-pavlov do you think it makes sense to add the guide to this repo?

[g-pavlov]

It's the contract between maintainers and contributors, so I guess it's logical to have it somewhere here: https://gardener.cloud/contribute/code

[vlerenc]

I was not really sure how to interpret the can/may/should/proposed when I read them a week ago, but when @rfranzke asked me about these meeting minutes today, we chatted and here a proposal what could be done.

It would be great, if you could review and then of course help with the implementation, if we are going to leverage the robot for that.

See: https://github.wdf.sap.corp/kubernetes/kube-backlog/issues/36

WDYT?

[zanetworker]

Write a pull request review guideline (take the points from #92 (comment)) and publish it alongside the pull request guideline

Why would the author of a PR read the review guideline, or did I mis-understand the first point?

Guideline how to improve effeciency with GitHub notifications and Outlook (and other e-mail clients), so that people do not turn off notifications or send them straight to the trash bin, e.g.:

too imperative IMO.

regarding the Bots proposals, as long as they require no additional actions from the reviewer (they do their thing without adding the overhead of knowing about / learning about each), it would LGTM :)

[vlerenc]

In regards to your points @zanetworker :

Why would the author of a PR read the review guideline, or did I mis-understand the first point?

The author would read the PR guide, e.g. whether the correct labels are set, that the PR tries to do only one thing, size/l and larger are, when possible, avoided, etc. See points above from your meeting minutes.

The reviewer would read the reviewer guide about responding timely, etc. See points above from your meeting minutes.

too imperative IMO.

That is just a recommendation, soft in nature, take it if it helps, but make reviewers aware that switching off notifications is not a good idea and if they feel overhelmed, here is some idea how to improve the situation for yourself.

they do their thing without adding the overhead of knowing about / learning about each

The idea behind the last "transparency" topics is to inform them passively. The active conformance is driven by the bots. In general the idea is, if they do their reviews, things shall work out. If they reviews are not coming by, the PR will not progress and people will see/know.

[vlerenc]

Hmm... no other feedback @stoyanr @g-pavlov @rfranzke @zanetworker @gardener/gardener-maintainers on https://github.com/gardener/documentation/issues/92#issuecomment-646072329? Does that make sense, no sense, do you want to have another meeting?

[zanetworker]

@vlerenc Sure, I think we could start simple considering your comment:

The author would read the PR guide, e.g. whether the correct labels are set, that the PR tries to do only one thing, size/l and larger are, when possible, avoided, etc. See points above from your meeting minutes.

I like this "The active conformance is driven by the bots" whenever possible. Maybe something like: https://github.com/google/triage-party or so.

So my proposal to move this forward, is whenver it makes sense lets edit and build on this, and file a PR.

@vlerenc For faster collab, I created https://docs.google.com/document/d/1I4DvefoHVsZZWyQmJ3BzHUzzCUwe3Pm4BI0n9J_T6IA/edit?usp=sharing

@ everyone feel free to add / remove / edit / reshape the document, then we can file a PR to the repos to with the final guideline.

[vlerenc]

Sure @zanetworker. If you prefer to continue the discussion in the doc, please copy https://github.com/gardener/documentation/issues/92#issuecomment-646072329 over. It was actually meant as a very concrete list of things that could help in this context and was a plan to execute if approved.

[zanetworker]

the doc is for the final PR, the discussion about what should be in the Doc or extra points should be tracked here. Similar to Kubernetes.

That said I don't have any pereference and I saw that things are not moving forward so I propsed the docs for faster formulation of the document itself (similar to proposals and Keps in Kubernetes before they reach final form)

[zanetworker]

regarding your list, no objections from my side :) But thats only regarding the bot right? not the review guide itself?

[vlerenc]

Moved to https://github.wdf.sap.corp/kubernetes/kube-backlog/issues/36.

[Kristian-ZH]

/invite @n-boshnakov

[gardener-robot]

@Kristian-ZH Command /invite is not allowed for issue but only for pullrequest.

[n-boshnakov]

Due to the issue being moved to a different repo, I will be closing it. Any comments regarding this issue should be made in the new repository.

[gardener-robot]

@n-boshnakov You have mentioned internal references in the public. Please check.