docs icon indicating copy to clipboard operation
docs copied to clipboard

Document permission required to `approve` ✅ a pull request

Open jsoref opened this issue 2 years ago • 10 comments

Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-roles-for-an-organization#permissions-for-each-role

What part(s) of the article would you like to see updated?

Permissions for each role lists many roles, including:

  • Merge pull requests on protected branches, even if there are no approving reviews

Oddly, it doesn't document approving itself.

Additional information

We have a repository where we wanted to give someone triage permission, it seemed like it should be enough for them to approve a PR w/o giving them write access.

Repository roles for organizations lists:

Triage: Recommended for contributors who need to proactively manage issues and pull requests without write access

Triage sounds like the right permission. And the general overview fits. But...

In testing, it appears that triage is insufficient to grant meaningful approval as shown here: image

Given that a repository can easily choose who has write access and who has triage, and who has neither, it doesn't seem unreasonable to expect that someone who could triage could also approve, w/o themselves being able to push directly to a repository.

That said. It is unreasonable to not be able to figure out from the documentation which role is necessary to actually approve.

jsoref avatar Sep 29 '22 12:09 jsoref

Uploading Screenshot_2022-09-22-20-21-22-37_40deb401b9ffe8e1df2f1cc5ba480b12.jpg…

eagleicatcher avatar Sep 29 '22 14:09 eagleicatcher

Everything

eagleicatcher avatar Sep 29 '22 14:09 eagleicatcher

👋 @jsoref - Thanks so much for opening an issue! I'll triage this for the team to take a look :eyes:

cmwilson21 avatar Sep 29 '22 14:09 cmwilson21

https://github.com/github/docs/issues/20989#issue-1390809757

SORA-ALONE avatar Sep 29 '22 14:09 SORA-ALONE

https://github.com/github/docs/issues/20989#issue-1390809757

SORA-ALONE avatar Sep 29 '22 14:09 SORA-ALONE

Code of Conduct

What article on docs.github.com is affected?

https://docs.github.com/en/organizations/managing-access-to-your-organizations-repositories/repository-roles-for-an-organization#permissions-for-each-role

What part(s) of the article would you like to see updated?

Permissions for each role lists many roles, including:

  • Merge pull requests on protected branches, even if there are no approving reviews

Oddly, it doesn't document approving itself.

Additional information

We have a repository where we wanted to give someone triage permission, it seemed like it should be enough for them to approve a PR w/o giving them write access.

Repository roles for organizations lists:

Triage: Recommended for contributors who need to proactively manage issues and pull requests without write access

Triage sounds like the right permission. And the general overview fits. But...

In testing, it appears that triage is insufficient to grant meaningful approval as shown here: image

Given that a repository can easily choose who has write access and who has triage, and who has neither, it doesn't seem unreasonable to expect that someone who could triage could also approve, w/o themselves being able to push directly to a repository.

That said. It is unreasonable to not be able to figure out from the documentation which role is necessary to actually approve.

reham31 avatar Oct 01 '22 16:10 reham31

Hi @jsoref, thanks for raising this! I just wanted to update you that I'm investigating this internally, and I'll get back to you as soon as I have something substantive.

subatoi avatar Oct 07 '22 14:10 subatoi

Hi @jsoref and apologies for the delay here... Looking at the table, there's a line:

Submit reviews that affect a pull request's mergeability

It's a tricky one: this makes sense because technically speaking a review by someone with less than Write access can be applied even if it's not a "mergeability-changing" review.

We've also got two articles: one and two that cover this.

That being said, I would say we could place the "affect mergeability" line next to the regular "Submit reviews" line to at least make this a little clearer.

subatoi avatar Oct 17 '22 11:10 subatoi

Just going to unassign myself from this as someone will pick up the PR review and continuing discussion will happen there. Thanks for raising that!

subatoi avatar Oct 20 '22 17:10 subatoi