Clarify the pull request review guidance for company diversity

[jerop]

As discussed at the governance/community meeting on 25/10, we clarify the guidance on company diversity for reviews. This update upholds that company diversity is an important recommendation but that maintainers have discretion to make exceptions after careful review of the changes.

Note that the policy for TEPs is different: a TEP must be approved by at least two owners from different companies

cc @wlynch @dibyom @pritidesai @tektoncd/governing-board

[QuanZhang-William]

Thanks for this update. This update looks good to me!

(and I think we are already strictly following the TEP review rules as well).

[jerop]

and I think we are already strictly following the TEP review rules as well.

Yes, the policy for TEPs is different: a TEP must be approved by at least two owners from different companies

[pritidesai]

This update upholds that company diversity is an important recommendation that should be observed
when appropriate, not a policy that is applied to all pull requests.

Have we seen company diversity recommendation applying to all pull requests?

We rely on maintainer discretion to determine when appropriate.

What is the best practice for the maintainers?

Is introducing almost 4000 lines of code with about 7 or 8 comments appropriate?

This is to ensure that we don't further slow down reviews in the project
when we have already gathered diverse input on the change in a TEP, or
the change is minor and reversible.

TEP only covers design proposal of a new feature. TEP does not cover how implementing that new feature can impact an existing functionality. This is generally discovered during an implementation phase. Not all functionalities are 100% covered by the automated tests and that's where reviewers play a key role in identifying any discrepancies.

[jerop]

This PR is a call for input and I encourage anyone to contribute wording that gets us to a place of clarity about our processes

This update upholds that company diversity is an important recommendation that should be observed
when appropriate, not a policy that is applied to all pull requests.

Have we seen company diversity recommendation applying to all pull requests?

What I have seen is contributors now worried about company diversity slowing down their pull requests, and even one considering writing one big pull request so that the delay doesn't add up across many small pull requests, so I wanted to ensure that it's not mistakenly interpreted to mean that we need to observe this guidance for all changes going forward

Is introducing almost 4000 lines of code with about 7 or 8 comments appropriate?

If you're referring to tektoncd/pipeline#7260, most of the lines of code were generated -- we are happy to address any feedback on the changes or revert the pull request if you'd like -- or was it another pull request?

Discretion will not always be the same among maintainers, I take ownership of any slip up in PRs I approved

TEP only covers design proposal of a new feature. TEP does not cover how implementing that new feature can impact an existing functionality. This is generally discovered during an implementation phase. Not all functionalities are 100% covered by the automated tests and that's where reviewers play a key role in identifying any discrepancies.

Agreed, that's why maintainers need to interpret when it's appropriate to gather more input, and it won't be perfect

Just trying to prevent a wrong interpretation of this guidance that will further slow down velocity in the project

[pritidesai]

If you're referring to tektoncd/pipeline#7260, most of the lines of code were generated -- we are happy to address any feedback on the changes or revert the pull request if you'd like -- or was it another pull request?

There has been many such PRs and that's the reason the guidelines were agreed on and put in place with #829 after a discussion with the community.

I am personally not advocating for reverting any changes introduced by tektoncd/pipeline#7260. The company policy was only raised here in this PR (introducing about 1900 lines of generated code out of 3800) to make sure other maintainers get an opportunity to review a very significant concept of the project which was merged under 48 hours.

We all are experiencing the velocity issue because of the lack of enforcement of reviewing PRs as they are opened. There are many factors contributing to such behavior - lack of topical ownership being the biggest.

We have been doing fairly good when PRs are part of a milestone and we should make that official if its not already to help with the velocity.

[afrittoli]

Thanks @jerop - this looks good to me.
/approve

[dibyom]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, dibyom, vdemeester

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [afrittoli,dibyom,vdemeester]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jerop]

Can we please merge this? 🙏🏾

[vdemeester]

/lgtm