gnodet-bot commented on code in PR #330:
URL:
https://github.com/apache/maven-gh-actions-shared/pull/330#discussion_r4181704001
##########
.github/workflows/pr-check.yml:
##########
@@ -71,6 +71,10 @@ jobs:
return;
}
+ // Values used by the job condition, logged for diagnosis
+ const { user, author_association } = context.payload.pull_request;
+ core.info(`Author: ${user.login}, type: ${user.type},
author_association: ${author_association}`);
Review Comment:
Good diagnostic line. One safety note: if this workflow were ever triggered
by an event that lacks a `pull_request` key in the payload, the destructuring
on L75 would throw (the optional chaining on L68 guards `prNumber` but not this
destructure). Not a problem today since L69–71 already bail out when `prNumber`
is falsy, but if someone ever rearranges the code, a guard or optional chaining
here would be more defensive.
Nit, not blocking.
##########
.github/workflows/pr-check.yml:
##########
@@ -85,6 +89,23 @@ jobs:
const { data: pr } = await github.rest.pulls.get({ owner, repo,
pull_number: prNumber });
const hasDeclaration = regex.test(pr.body || '');
+ // Committers don't need the declaration.
+ // Note: private org members appear as CONTRIBUTOR in the event
payload,
+ // so we check write access via the API instead of relying on
author_association.
+ let hasWriteAccess = false;
+ try {
+ const { data: perm } = await
github.rest.repos.getCollaboratorPermissionLevel({
+ owner, repo, username: pr.user.login,
+ });
+ hasWriteAccess = ['admin', 'write'].includes(perm.permission);
Review Comment:
The API can also return `maintain` as a distinct permission value (see
[docs](https://docs.github.com/en/rest/collaborators/collaborators#get-repository-permissions-for-a-user)).
The docs say:
> *The `permission` attribute provides the legacy base roles of `admin`,
`write`, `read`, and `none`, where the `maintain` role is mapped to `write` and
the `triage` role is mapped to `read`.*
So in practice `maintain` is mapped to `write` by the API and this check is
fine. But other projects (e.g. [fastmcp](https://github.com/PrefectHQ/fastmcp),
[honcho](https://github.com/plastic-labs/honcho)) include `maintain` explicitly
for safety. If you want to be extra defensive:
```suggestion
hasWriteAccess = ['admin', 'maintain',
'write'].includes(perm.permission);
```
Optional — the current code works correctly because of the legacy mapping.
##########
.github/workflows/pr-check.yml:
##########
@@ -85,6 +89,23 @@ jobs:
const { data: pr } = await github.rest.pulls.get({ owner, repo,
pull_number: prNumber });
const hasDeclaration = regex.test(pr.body || '');
+ // Committers don't need the declaration.
+ // Note: private org members appear as CONTRIBUTOR in the event
payload,
+ // so we check write access via the API instead of relying on
author_association.
+ let hasWriteAccess = false;
+ try {
+ const { data: perm } = await
github.rest.repos.getCollaboratorPermissionLevel({
+ owner, repo, username: pr.user.login,
+ });
+ hasWriteAccess = ['admin', 'write'].includes(perm.permission);
+ if (!hasWriteAccess) {
+ core.info(`Permission of ${pr.user.login}:
${perm.permission}`);
+ }
Review Comment:
The `core.info` only fires when `hasWriteAccess` is `false`. When it's
`true`, the permission level is never logged — the success message on L156
mentions write access but not which level (`admin` vs `write` vs `maintain`).
Consider logging unconditionally for better diagnostics:
```suggestion
core.info(`Permission of ${pr.user.login}:
${perm.permission}`);
```
Minor nit — the current form works, you just lose visibility on the exact
role for committers.
##########
.github/workflows/pr-check.yml:
##########
@@ -85,6 +89,23 @@ jobs:
const { data: pr } = await github.rest.pulls.get({ owner, repo,
pull_number: prNumber });
const hasDeclaration = regex.test(pr.body || '');
+ // Committers don't need the declaration.
+ // Note: private org members appear as CONTRIBUTOR in the event
payload,
Review Comment:
Good comment. The term `CONTRIBUTOR` here is accurate — private org members
whose only relationship to the repo is through org membership (not an explicit
collaborator invite) do appear as `CONTRIBUTOR` in `author_association` (per
[michaelheap.com](https://michaelheap.com/github-actions-check-permission/) and
[GitHub community
discussion](https://github.com/orgs/community/discussions/78038)). The API
permission check correctly covers this gap. 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
