gnodet commented on code in PR #330:
URL:
https://github.com/apache/maven-gh-actions-shared/pull/330#discussion_r4179155854
##########
.github/workflows/pr-check.yml:
##########
@@ -92,6 +89,17 @@ 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 - checked here, as
private org members are not MEMBER in the event payload
+ let hasWriteAccess = false;
+ try {
+ const { data: perm } = await
github.rest.repos.getCollaboratorPermissionLevel({
+ owner, repo, username: pr.user.login,
+ });
Review Comment:
`getCollaboratorPermissionLevel()` returns `permission` as one of `admin`,
`write`, `read`, `none` — `write` covers committers (push access) and `admin`
covers org owners. This is correct.
One concern: if the API call fails (403 because the token lacks
`members:read`, or the user is not a collaborator), `hasWriteAccess` stays
`false` and the check proceeds normally — that's acceptable fallback behavior.
But perhaps a committer-opened PR failing because of a token permission issue
would be confusing. Worth noting in the comment.
##########
.github/workflows/pr-check.yml:
##########
@@ -78,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:
`context.payload.pull_request` is `undefined` inside a reusable workflow
called via `workflow_call` — this is exactly the root cause we diagnosed (it
throws `TypeError: Cannot read properties of undefined (reading
'pull_request')`).
Since `pr` is already fetched from the API a few lines below, this whole
block should be moved after the `pulls.get()` call and read from `pr` instead:
```suggestion
// Values used by the job condition, logged for diagnosis
core.info(`Author: ${pr.user.login}, type: ${pr.user.type},
author_association: ${pr.author_association}`);
```
##########
.github/workflows/pr-check.yml:
##########
@@ -92,6 +89,17 @@ jobs:
const { data: pr } = await github.rest.pulls.get({ owner, repo,
pull_number: prNumber });
const hasDeclaration = regex.test(pr.body || '');
Review Comment:
This block should be placed **before** the `regex` / `hasDeclaration` check
(which is already the case), but also before fetching comments — there's no
point iterating through all PR comments if we're going to skip anyway. Consider
restructuring as an early return:
```suggestion
// 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;
```
--
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]
