Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename Reviewer role to Collaborator #982

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dibyom
Copy link
Member

@dibyom dibyom commented Mar 17, 2023

Our GitHub teams use the term collaborator to maintain the list of reviewers which can be confusing. This commit renames the Reviewer role to Collaborator to match the way our team setup works.

/cc @tektoncd/governing-board

@tekton-robot
Copy link
Contributor

@dibyom: GitHub didn't allow me to request PR reviews from the following users: tektoncd/governing-board.

Note that only tektoncd members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

Our GitHub teams use the term collaborator to maintain the list of reviewers which can be confusing. This commit renames the Reviewer role to Collaborator to match the way our team setup works.

/cc @tektoncd/governing-board

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@tekton-robot tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 17, 2023
@lbernick
Copy link
Member

Curious how annoying it would be to rename "collaborator" in our org to "reviewer" instead? I think "reviewer" is a clearer description of what the role is. But if it's a lot easier to just rewrite the markdown docs I think this is fine.

@dibyom
Copy link
Member Author

dibyom commented Mar 21, 2023

Curious how annoying it would be to rename "collaborator" in our org to "reviewer" instead?

I think we'd have at least change our team names and any automation that depends on it. I slightly prefer collaborator since IMO there's more (or should be more) to the role than just LGTM'ing PRs e.g. triaging issues, build failures etc.

@dibyom
Copy link
Member Author

dibyom commented Mar 21, 2023

/cc @wlynch @afrittoli @jerop @vdemeester

@lbernick
Copy link
Member

Curious how annoying it would be to rename "collaborator" in our org to "reviewer" instead?

I think we'd have at least change our team names and any automation that depends on it. I slightly prefer collaborator since IMO there's more (or should be more) to the role than just LGTM'ing PRs e.g. triaging issues, build failures etc.

sgtm, looking at the ladder again I think you're right that these extra responsibilities are reflected

/lgtm

@tekton-robot tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 21, 2023
Copy link
Member

@jerop jerop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me, but want to make sure other governing board members have a chance to review

/hold

@tekton-robot tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 21, 2023
@wlynch
Copy link
Member

wlynch commented Mar 22, 2023

We should probably add another section for how collaborator / maintainer relate to reviewer / approvers.

Being able to have a separation between the roles is nice because there is a way let people be able to LGTM things without needing to be on the active reviewer list. We also have similar behavior with maintainer / approver - e.g. collaborators can be granted approver permissions, but that doesn't make them maintainers w.r.t. the contributor ladder.

I think this is a step in the right direction, but right now this somewhat reads as "if you are collaborator you are expected to be an active reviewer", which I don't think is what we're aiming for.

@dibyom
Copy link
Member Author

dibyom commented Mar 24, 2023

reviewers/approvers as the prow approve plugin uses it right?

Being able to have a separation between the roles is nice because there is a way let people be able to LGTM things without needing to be on the active reviewer list.

How does one LGTM things without being on the reviewer list? Are you referring to the prow plugin that auto-assigns reviewers or something else?

We also have similar behavior with maintainer / approver - e.g. collaborators can be granted approver permissions, but that doesn't make them maintainers w.r.t. the contributor ladder.

The ladder already says collaborators " Can be allowed to /approve pull requests in specific
sub-directories of a project (by maintainer discretion)".

I think this is a step in the right direction, but right now this somewhat reads as "if you are collaborator you are expected to be an active reviewer", which I don't think is what we're aiming for.

Depends on what you mean by active, but I do think we are trying to encourage collaborators to help out with reviews in general. I'll put this on the next gov board meeting so we can discuss more

@wlynch
Copy link
Member

wlynch commented Mar 24, 2023

How does one LGTM things without being on the reviewer list? Are you referring to the prow plugin that auto-assigns reviewers or something else?

You just need explicit read access on a repo to be able to /lgtm.

We used to let anyone in the org be able to /lgtm PRs, even if they did were not explicitly a reviewer on the repo (this is what k8s does). Reviewer was just a role that meant you would get auto-assigned PRs and there were expectations around triaging issues, etc.

This would let people hop in and add reviews, even if they weren't formally in a reviewer role.

I don't have the context / backstory around why we removed it, though it looks like it was an intentional decision. 🤷

The ladder already says collaborators " Can be allowed to /approve pull requests in specific sub-directories of a project (by maintainer discretion)".\

Yup! Exactly. What I'm getting at is there seems to be a separation around permissions and roles that we're not really capturing in the ladder. The same way you can /approve without being a maintainer, it'd be great if we had a way to /lgtm with needing to be a reviewer. I used to use reviewer vs collaborator for this distinction, but if we're folding these together idk how we should distinguish this moving forward. (open to ideas)

Depends on what you mean by active, but I do think we are trying to encourage collaborators to help out with reviews in general. I'll put this on the next gov board meeting so we can discuss more

👍 Thanks!

@afrittoli
Copy link
Member

How does one LGTM things without being on the reviewer list? Are you referring to the prow plugin that auto-assigns reviewers or something else?

You just need explicit read access on a repo to be able to /lgtm.

We used to let anyone in the org be able to /lgtm PRs, even if they did were not explicitly a reviewer on the repo (this is what k8s does). Reviewer was just a role that meant you would get auto-assigned PRs and there were expectations around triaging issues, etc.

This would let people hop in and add reviews, even if they weren't formally in a reviewer role.

I don't have the context / backstory around why we removed it, though it looks like it was an intentional decision. 🤷

Individuals who are not part of the org will need "ok-to-test" on their PRs and will not be able to assign themselves issues.
However, adding someone to the org would mean granting them LGTM on any repo in the tektoncd.
To solve this issue, we removed the default "read" permission on repos (they are public anyways) which prevents people to gain overall lgtm power, and introduced collaborators team, which allow for more granularity.

The ladder already says collaborators " Can be allowed to /approve pull requests in specific sub-directories of a project (by maintainer discretion)".\

Yup! Exactly. What I'm getting at is there seems to be a separation around permissions and roles that we're not really capturing in the ladder. The same way you can /approve without being a maintainer, it'd be great if we had a way to /lgtm with needing to be a reviewer. I used to use reviewer vs collaborator for this distinction, but if we're folding these together idk how we should distinguish this moving forward. (open to ideas)

Depends on what you mean by active, but I do think we are trying to encourage collaborators to help out with reviews in general. I'll put this on the next gov board meeting so we can discuss more

👍 Thanks!

@afrittoli
Copy link
Member

Since repositories and PRs are public, literally everyone with a GitHub account may review code, leave comments and say "LGTM", everyone's comments are taken into account.

At least for the pipeline project, /lgtm is one of the two votes required, along with /approve, to merge code, and I expect both votes to be cast by individuals who are familiar with the codebase, who are regular contributors two the projects and have proven their interest in the project and its neutrality - that's not necessarily the case for everyone in the org.

We could consider the alternatives below, but honestly, I don't like either of them:

  • raise the bar for entrance to the org. This brings back the issue with contributors being able to assign themselves issues.
  • require two "approve" for PRs. I don't like this because it reduces the importance of the lgtm vote and thus the role of reviewers

@vdemeester
Copy link
Member

  • raise the bar for entrance to the org. This brings back the issue with contributors being able to assign themselves issues.

I think anyone can assign itself to an issue as long as they commented once on it 🤔

@tekton-robot
Copy link
Contributor

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 22, 2023
@tekton-robot
Copy link
Contributor

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.

@tekton-robot tekton-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 21, 2023
@tekton-robot tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 18, 2023
@vdemeester
Copy link
Member

@dibyom @wlynch @afrittoli what's the status on this one ?

@dibyom
Copy link
Member Author

dibyom commented Nov 15, 2023

@vdemeester thanks for the ping, let me revive this

@dibyom dibyom removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Nov 15, 2023
Our GitHub teams use the term `collaborator` to maintain the list of
`reviewers` which can be confusing. This commit renames the Reviewer role to
Collaborator to match the way our team setup works.

Signed-off-by: Dibyo Mukherjee <[email protected]>
@tekton-robot tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2023
@tekton-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@tekton-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jerop

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:

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

@tekton-robot tekton-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants