Change merge policy

[durandom]

I propose to implement a review and merge process, similar to what we have in https://github.com/operate-first/ and discussed in operate-first/apps#81

This would encompass:

• add merge permissions to @sesheta - https://github.com/AICoE/aicoe-ci#github-chatops-options
• add approval permissions to @open-infrastructure-labs/operations
• require 2 approvals
• branch protection for the master/main branch

I'm not sure though if 2 approvals and bot merge can work - that could be circumvented by granting merge permissions to the @open-infrastructure-labs/operations team

This would be applicable to

• https://github.com/open-infrastructure-labs/moc-cnv-sandbox
• ???

[anishasthana]

That's how we currently do things in a number of other repos -- https://github.com/AICoE/idh-manifests/ as an example. 2 approvals and bot merge can work together without issue.

[larsks]

Does @sesheta require "Write" or "Maintain" access to the repository?

[goern]

I question the 'two approvals' policy. I could imagine two roles: a reviewer (responsible for looking thru the PR making sure its good) and an582047410458 approver (basically depending on the reviewer and taking the accountability to merge the code)

If you envision a larger group of reviewers taking the majority of work, the approver is the one coordinating feature release, as it might require cross repo dependencies, or even constraints that are not documented as PR...

[durandom]

I'm also good with one approval and multiple reviewers. Do the bots already support this workflow?
Otherwise we could start with 2 approvals and change later.
One goal is to speed up the merge process

[tumido]

@goern How does the bots treat a GitHub review though?

For the context, I like to review via the GitHub reviews, because then my review can be re-requested etc/cleared whenever needed and I prefer it over the /approve and /lgtm commands since I want to see who approved what and how many people actually saw it - like our ADRs.

If I "approve" something via a Github review and I'm listed as both Prow "approver" and Prow "reviewer" in that repo, does it mean that I've approved the PR and it will be auto-merged since I'm an approver? Therfore it would act and merge it even though only a single person saw it?

At some (auto-deployed) repos, I want at least 2 people to see a PR. The GitHub approval setting to 2 approves physically blocks people to merge something with a single review. I like that. I rather ping people to get a second pair of eyes on a PR than allow this to be bypassed. I like this to be a hard requirement, so we really have people engaged in the review process, so more than 1 or 2 people really knows what actually is in that particular repo (bus factor > 2).

[anishasthana]

++ @tumido

[goern]

for https://prow.operate-first.cloud/ a GitHub approving review is equivalent to a /approve comment, see https://github.com/thoth-station/thoth-application/blob/master/prow/overlays/cnv-prod/plugins.yaml#L80-L87 and https://godoc.org/k8s.io/test-infra/prow/plugins#Approve So the difference of a Prow approver and a GitHub reviewer is the power distribution: OR vs AND

Prow (more specifically to LGTM and approve plugins) try to distinguish by role: if reviewers are happy, the approver should be so confident, that he can simply click the button. so the real hard review work will finish with an /lgtm Depending solely on Prow to do all the review/approve/merge work would enable us to have all the srcops configuration in prow's yaml files...

[goern]

.. had a short call with @tumido here is our proposal and goal. We would like to experiment with the proposal on the /support repo tomorrow...

Goal

help the majority fo the team to do good code reviews and take responsibility for high-quality code. GitHub UI and Prow /-commands should enable the same workflows for the same kind of members.

Proposal

1. create a Devs team on GitHub that is able to do 'triage' on support/, see https://docs.github.com/en/github/setting-up-and-managing-organizations-and-teams/repository-permission-levels-for-an-organization
2. sort the OWNERS file, so that @tumido and @HumairAK are the approvers, the reviewers are all the members of the Devs team
3. BTW, approvers are responsible to help reviewers doing a good job, see Goal ;)

[goern]

BTW, operate-first/apps#152 and AICoE/idh-manifests#77 and good examples of non-obvious configurations of GitHub and Prow that look fancy...

[tumido]

Just to add a bit more meat to it. The proposal should allow us:

• We can do reviews in Github UI. If the GH review results in selecting the Approve action, it should translate to a lgtm label being applied by Prow
• If a GH review result in requested changes, Prow is not touching a thing and a review can get re-requested as we are used to from the GH UI.
• Reviewers can't merge, the GitHub button is disabled for them.
• Approvers can merge manually, but it should be only a failsafe mechanism, it is preferred for them to merge via /approve command
• Approvers heavily rely on reviewers reviewing PRs
• Approvers can review via GitHub - the GitHub approve action should translate to a lgtm label being applied as well.
• Only when an approver commands Prow to /approve the PR gets merged. No matter how many Github reviews with green tick a PR has, the approver must initiate the action.
• The number of reviewers is much higher than approvers (only ~2 people can approve, many can review)
• The OWNERS file is context aware, so we can have a different set of approvers for different paths/folders within repository. Therefore, for example in the operate-first/apps repository, each application can have a different set of approvers.

This should be rewritten as an ADR once we agree on the direction.

[HumairAK]

This definitely seems like the most appropriate way of doing things. I guess my biggest concern is PRs getting bottlenecked by the specific set of approvers (we're still in relatively early stages where we're trying to move a bit rapidly). Separating out approvers by location, should help with this I'd imagine. But let's try it out, and see how it goes.

[HumairAK]

@tumido
With regards to:

The OWNERS file is context aware, so we can have a different set of approvers for different paths/folders within repository. Therefore, for example in the operate-first/apps repository, each application can have a different set of approvers.

Say if I have this scenario:

• odh/monitoring has approvers x and y
• odh/kafka has approvers a and b

If a pr changes odh/monitoring and odh/kafka then do all x,y,a, and b become approvers ?