tektoncd
Rename Reviewer role to Collaborator
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
@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
collaboratorto maintain the list ofreviewerswhich 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.
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.
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.
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
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.
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
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
/approvepull 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!
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
readaccess on a repo to be able to/lgtm.We used to let anyone in the org be able to
/lgtmPRs, 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
/approvepull 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
/approvewithout being a maintainer, it'd be great if we had a way to/lgtmwith 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!
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
- 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 🤔
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.
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.
[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
- ~~OWNERS~~ [jerop]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
@dibyom: PR needs rebase.
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.