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

Don't fetch all users/teams to check reviewers #159

Merged
merged 1 commit into from
Apr 23, 2023

Conversation

andrewhamon
Copy link
Contributor

@andrewhamon andrewhamon commented Apr 2, 2023

Prior to this PR, reviewers were validated by first fetching all teams and users, and then checking membership in that result. This suffers from a few issues:

  • it was broken since it seemed to only fetch one page, not all pages. This can't work with any significant number of users or teams in an organization.
  • it is inefficient
    • fetching the full list is not necessary
    • there was a 1+n style query, where the users name would be looked for each user in the list.

This PR modifies reviewer validation by fetching only the users/teams specified in the commit.

Prior to this PR, reviewers were validated by first fetching all teams
and users, and then checking membership in that result. This suffers
from a few issues:

- it was broken since it seemed to only fetch one page, not all pages
- it is inefficient
  - fetching the full list is not necessary
  - there was a 1+n style query, where each user returned would then
    have their name fetched one by one

This PR modifies reviewer validation by fetching only the users/teams
specified in the commit.
@andrewhamon andrewhamon changed the title Don't fetch all users and teams to check reviewers Don't fetch all users/teams to check reviewers Apr 2, 2023
Copy link
Owner

@spacedentist spacedentist left a comment

Choose a reason for hiding this comment

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

Nice work! Thanks a lot!

@spacedentist spacedentist merged commit a2c6a81 into spacedentist:master Apr 23, 2023
clarityflowers pushed a commit to clarityflowers/spr that referenced this pull request Mar 19, 2024
Prior to this PR, reviewers were validated by first fetching all teams
and users, and then checking membership in that result. This suffers
from a few issues:

- it was broken since it seemed to only fetch one page, not all pages.
This can't work with any significant number of users or teams in an
organization.
- it is inefficient
  - fetching the full list is not necessary
- there was a 1+n style query, where the users name would be looked for
each user in the list.

This PR modifies reviewer validation by fetching only the users/teams
specified in the commit.

Reviewers: spacedentist

Reviewed By: spacedentist

Pull Request: spacedentist#159
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