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

[Dev] PR Workflow incorrectly tagging committer reviews that have their Apache membership set to private #34381

Closed
raulcd opened this issue Feb 28, 2023 · 6 comments · Fixed by #34557

Comments

@raulcd
Copy link
Member

raulcd commented Feb 28, 2023

Describe the bug, including details regarding any error messages, version, and platform.

Committer GitHub role is CONTRIBUTOR instead of MEMBER.
Example of event payload for event triggered from Committer:

"author_association": "CONTRIBUTOR",

Component(s)

Developer Tools

@raulcd
Copy link
Member Author

raulcd commented Feb 28, 2023

That is not exactly true. It seems to happen only for some committers. Maybe there's something wrong on the GitHub permissions?
For example we can see the event payload on a review made from me on this PR:
https://github.com/apache/arrow/actions/runs/4293861653
The author_association on the PR review comment is MEMBER but these two (made by @pitrou and @AlenkaF ) appear as if the review comment author_association is CONTRIBUTOR:
https://github.com/apache/arrow/actions/runs/4293849744
https://github.com/apache/arrow/actions/runs/4293586215
edited for a typo

@raulcd
Copy link
Member Author

raulcd commented Feb 28, 2023

As pointed out by @assignUser this might have to do with some users setting their Apache role to private (https://github.com/orgs/apache/people) similar to: flutter/flutter#101012

@raulcd raulcd changed the title [Dev] PR Workflow incorrectly tagging for committer reviews [Dev] PR Workflow incorrectly tagging committer reviews that have their Apache membership set to private Feb 28, 2023
@raulcd
Copy link
Member Author

raulcd commented Mar 1, 2023

I have tested giving all possible read permissions to the workflow and requesting via API instead of using the default payload event using the secrets.GITHUB_TOKEN. This token does not have permissions to read the organization members and if the member is private it won't appear as MEMBER.
I've done a test adding a new token with only read:org permissions which we could use to retrieve the membership. Something like the following:

    def is_committer(self, review=False):
        # We require a new connection to GitHub with a specific token to read
        # membership association.
        member_conn = github.Github(self._member_token)
        repo = member_conn.get_repo(self.event_payload['repository']['id'], lazy=True)
        gh_obj = repo.get_pull(self.event_payload['pull_request']['number'])
        if review:
            gh_obj = gh_obj.get_review(self.event_payload['review']['id'])
        author_association = gh_obj.raw_data['author_association']
        print(f"Author Association is {author_association}")
        return author_association in COMMITTER_ROLES

This would work but would require to set up a new secret on our repo, something like MEMBERSHIP_GITHUB_TOKEN.
The only other solution I can think of is to have a list of committers and check the author name is on the list but that would require maintaining a list of committers.
@kou @assignUser any preference on any of the two solutions or other ideas you think I could try?

@kou
Copy link
Member

kou commented Mar 2, 2023

We already have a commiter list: https://github.com/apache/arrow-site/blob/main/_data/committers.yml
Can we use this?

@assignUser
Copy link
Member

Oh nice, as the list already exists and is even in yaml it should be easy to use!

@raulcd
Copy link
Member Author

raulcd commented Mar 2, 2023

Thanks @kou I forgot this existed. Yes that should work. I'll take the committer GitHub usernames from there.

raulcd added a commit to raulcd/arrow that referenced this issue Mar 14, 2023
…yml instead of relying on author_association
@kou kou added this to the 12.0.0 milestone Mar 15, 2023
kou added a commit that referenced this issue Mar 15, 2023
…stead of relying on author_association (#34557)

### Rationale for this change

If a committer has their ASF role on GitHub as private the GitHub PR bot author_association is not correctly assigned.

### What changes are included in this PR?

 This change uses the committers list on the arrow_site repository to retrieve the list of committers.

### Are these changes tested?

There is a unit test and have tested the different steps of the workflow individually but haven't tested the full workflow.

### Are there any user-facing changes?

No
* Closes: #34381

Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
rtpsw pushed a commit to rtpsw/arrow that referenced this issue Mar 27, 2023
…yml instead of relying on author_association (apache#34557)

### Rationale for this change

If a committer has their ASF role on GitHub as private the GitHub PR bot author_association is not correctly assigned.

### What changes are included in this PR?

 This change uses the committers list on the arrow_site repository to retrieve the list of committers.

### Are these changes tested?

There is a unit test and have tested the different steps of the workflow individually but haven't tested the full workflow.

### Are there any user-facing changes?

No
* Closes: apache#34381

Lead-authored-by: Raúl Cumplido <raulcumplido@gmail.com>
Co-authored-by: Sutou Kouhei <kou@cozmixng.org>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment