Skip to content

skip reviewers who are no longer in the organization#442

Open
aadc-dev wants to merge 4 commits intomainfrom
aadcdev/backportReviewersValidOrgMembers
Open

skip reviewers who are no longer in the organization#442
aadc-dev wants to merge 4 commits intomainfrom
aadcdev/backportReviewersValidOrgMembers

Conversation

@aadc-dev
Copy link
Contributor

@aadc-dev aadc-dev commented Jan 28, 2026

When assigning backport reviewers, the bot mirrors the original PR's assigned reviewers and those who submitted reviews. This change updates the logic to filter out users who have since left the organization.

Implementation fetches all current org members, currently sitting at 175, which will require two requests (each call at 100 per), which on average should be less than making individual calls to IsOrgMember per backport reviewer.

…total members, needing two requests, which is less than doing one per reviewer on backports
@aadc-dev aadc-dev requested a review from a team as a code owner January 28, 2026 23:00
@doggydogworld
Copy link
Contributor

There is one small implementation detail that needs to be considered. The current API you are using is to list all organization members but there is also an API available to check organization membership for a single user. I assume it's to reduce the amount of API calls we make to avoid rate limiting issues since at most always make 2 calls vs potentially more calls using the individual API. It should be documented just so that we don't have questions later related to that.

Another thing that should be documented as it may result in false-negatives is that the API needs certain permissions. It's not documented very well in GitHub docs but different permissions may have result in different successful results for this call. e.g. it may be the case that some users only get public membership while other uses can get both public/private membership.

return nil, trace.Wrap(err)
}

memberLoginToIsExistingOrgUser := make(map[string]bool)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Use empty struct instead since. It's a bit more idiomatic and suggests that the underlying value is not important only the key is.

Suggested change
memberLoginToIsExistingOrgUser := make(map[string]bool)
memberLoginToIsExistingOrgUser := make(map[string]struct{})

Copy link
Contributor

@doggydogworld doggydogworld left a comment

Choose a reason for hiding this comment

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

LGTM just a small nit and some comments related to documenting decisions and potential failures.

@aadc-dev
Copy link
Contributor Author

aadc-dev commented Feb 3, 2026

There is one small implementation detail that needs to be considered. The current API you are using is to list all organization members but there is also an API available to check organization membership for a single user. I assume it's to reduce the amount of API calls we make to avoid rate limiting issues since at most always make 2 calls vs potentially more calls using the individual API. It should be documented just so that we don't have questions later related to that.

Another thing that should be documented as it may result in false-negatives is that the API needs certain permissions. It's not documented very well in GitHub docs but different permissions may have result in different successful results for this call. e.g. it may be the case that some users only get public membership while other uses can get both public/private membership.

Yup, my first commit used the API to check a single user for Org membership, I then looked to confirm how many users we actually have in our org, got 175, and made the change to do bulk search.
But yes, will add more info into the function's godoc, as we may want to update after a certain threshold of engineers in our org.

…is is preferable than individial calls to isOrgMember as our org currently has about 175 members, requiring two calls, at 100 per call, when most PRs have more than two reviewers
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.

2 participants