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

docs: updates triager role to reviewer role in contributor ladder #70

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jpower432
Copy link
Member

@jpower432 jpower432 commented Aug 26, 2024

What Changed?

Per feedback during GitHub Teams structure review with compliance-trestle:

  • Replace triager with reviewer

Rationale

The Triager role has the responsibility of reviewing PR and issues without access to the source repository.

A specific issue cited here from GitHub Docs:
If a collaborator with admin, owner, or write access to the repository submits a review requesting changes, the pull request cannot be merged until the same collaborator submits another review approving the changes in the pull request.

A triager would not have blocking feedback and cannot merge PRs that are fully approved.

Copy link

@AleJo2995 AleJo2995 left a comment

Choose a reason for hiding this comment

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

LGTM - everything makes sense to me

MEMBERSHIP.md Outdated

* You have the permission to approve, but not merge, a PR.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can reviewer not merge a PR? Above in the permissions for Reviewer it is mentioned that they have write access with merging restricted via CODEOWNERS.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vikas-agarwal76 I think this could be reworded, but the reviewer could merge a PR (they would have write access), it would just need a maintainer approval first if "Require review from Code Owners" is set as a branch protection rule. Does this clarify? If so, I can alter the sections pertaining to this.

Copy link
Member Author

@jpower432 jpower432 Aug 27, 2024

Choose a reason for hiding this comment

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

I refined the section to be more explicit about what would be restricted through CODEOWNERS

MEMBERSHIP.md Outdated

Triagers are active contributors in the community through issue and pull request triage. Triagers are expected to remain active in this task.
Reviewers are knowledgeable about both the codebase and are able review code for quality and correctness. They should expect issues and pull requests (PRs) to be assigned to them and respond per community expectations.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove "both"?

@@ -105,7 +120,7 @@ As a project Maintainer, you have the following responsibilities and privileges:
* You make and approve technical design decisions.
* You set technical direction and priorities.
* You define milestones and releases.
* You mentor and guide contributors to the project, including mentoring and sponsoring potential Triager and Maintainer candidates.
* You mentor and guide contributors to the project, including mentoring and sponsoring potential Reviewer and Maintainer candidates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Reviewer mentors Maintainer candidates?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is listed as a Maintainer responsibility. Existing maintainers mentor reviewer and maintainer candidates.

We want maintainer approval to merge PRs, but not restrict
the ability to merge PRs with the proper approvals

Signed-off-by: Jennifer Power <[email protected]>
Copy link
Member

@jflowers jflowers left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@vikas-agarwal76 vikas-agarwal76 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@degenaro degenaro left a comment

Choose a reason for hiding this comment

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

LGTM

@jpower432
Copy link
Member Author

Looks like this change is approved. I propose that we demonstrate this in compliance-trestle prior to merging through this issue oscal-compass/compliance-trestle#1716

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants