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

Allow preferred docs reviewers #179

Merged
merged 2 commits into from
Nov 29, 2023
Merged

Conversation

ptgott
Copy link
Contributor

@ptgott ptgott commented Nov 10, 2023

The code reviewer configuration allows us to name preferred reviewers for certain file paths. For docs PRs, the workflow bot does not consult the preferred reviewer configuration. This means that, even if we add the preferredReviewerFor key to a codeReviewers entry with a docs path as a value, the workflow bot continues to add only docsReviewers to docs PRs.

In this change, the workflow bot gets docs reviewers by:

  1. Retrieving the docs reviewer pool from the config (like it does now).
  2. Retrieving all preferred reviewers for the file paths included within a PR. Since the pool of dedicated docs reviewers is small, the workflow bot gets all preferred reviewers for a file path, not just one.
  3. Merging the two lists of reviewers.

The alternative would be to allow preferred file paths in the docs reviewer pool, but it makes sense to have a small, consistent set of docs reviewers who have experience with docs reviews and a larger pool of subject matter experts for specific file paths from the code reviewer pool.

Implementation notes:

  • Adds an *Assignments.getAllPreferredReviewers method, which fetches all preferred reviewers for all files in a change set, rather than limiting the number of reviewers per file to 1 as in getPreferredReviewers. This is necessary for CheckInternal to check that code reviewers with expertise over specific docs paths are counted as valid docs reviewers.
  • Changes getDocsReviewers, CheckInternal, and checkInternalDocsReviews to take a slice of PullRequestFiles so they can apply the getPreferredReviewers logic.

@ptgott ptgott requested review from a team November 10, 2023 20:15
@ptgott
Copy link
Contributor Author

ptgott commented Nov 10, 2023

For context, this github-terraform PR adds preferred reviewers for docs, but it looks like we need to edit the workflow bot for this change to take effect: https://github.com/gravitational/github-terraform/pull/694/files

@ptgott ptgott force-pushed the paul.gottschling/2023-11-10-docs-pref branch from 5ef6bda to 5390557 Compare November 13, 2023 22:56
bot/internal/review/review.go Outdated Show resolved Hide resolved
if !strings.HasPrefix(file.Name, path) {
continue
}
preferredReviewers = append(preferredReviewers, reviewers...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels like this can result in the same reviewer being added multiple times - is this okay?

Copy link
Contributor Author

@ptgott ptgott Nov 16, 2023

Choose a reason for hiding this comment

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

Oversight on my part! I've changed this to deduplicate preferredReviewers. The triple for loop isn't the greatest (it also assumes the number of file paths and reviewers per path remains small), so I've explained the logic in a comment.

bot/internal/review/review.go Show resolved Hide resolved
bot/internal/review/review.go Show resolved Hide resolved
bot/internal/review/review.go Show resolved Hide resolved
@ptgott ptgott requested a review from zmb3 November 16, 2023 19:59
The code reviewer configuration allows us to name preferred reviewers
for certain file paths. For docs PRs, the workflow bot does not consult
the preferred reviewer configuration. This means that, even if we add
the `preferredReviewerFor` key to a `codeReviewers` entry with a docs
path as a value, the workflow bot continues to add only `docsReviewers`
to docs PRs.

In this change, the workflow bot gets docs reviewers by:

1. Retrieving the docs reviewer pool from the config (like it does now).
1. Retrieving all preferred reviewers for the file paths included within
   a PR. Since the pool of dedicated docs reviewers is small, the
   workflow bot gets all preferred reviewers for a file path, not just
   one.
1. Merging the two lists of reviewers.

The alternative would be to allow preferred file paths in the docs
reviewer pool, but it makes sense to have a small, consistent set of
docs reviewers who have experience with docs reviews and a larger pool
of subject matter experts for specific file paths from the code reviewer
pool.

Implementation notes:

- Adds an `*Assignments.getAllPreferredReviewers` method, which fetches
  all preferred reviewers for all files in a change set, rather than
  limiting the number of reviewers per file to 1 as in
  `getPreferredReviewers`. This is necessary for `CheckInternal` to
  check that code reviewers with expertise over specific docs paths are
  counted as valid docs reviewers.
- Changes `getDocsReviewers`, `CheckInternal`, and
  `checkInternalDocsReviews` to take a slice of `PullRequestFile`s so
  they can apply the `getPreferredReviewers` logic.
- Fix spelling.
- Add log if admin reviewers are assigned.
- Deduplicate preferred reviewers.
@ptgott ptgott force-pushed the paul.gottschling/2023-11-10-docs-pref branch from 860c394 to fc146a1 Compare November 28, 2023 21:43
@ptgott ptgott merged commit b7d826b into main Nov 29, 2023
7 checks passed
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