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

bot: Fix cloud reviewer assignments #212

Merged
merged 5 commits into from
Feb 8, 2024
Merged

bot: Fix cloud reviewer assignments #212

merged 5 commits into from
Feb 8, 2024

Conversation

jimbishopp
Copy link
Contributor

@jimbishopp jimbishopp commented Jan 8, 2024

Fix an issue with cloud repo assignments, remove the Team field, and add a preferredOnly flag.

Context: our reviewer data model was changed last month to allow core team members to review code in the cloud repository. The new team maps (CoreReviewers and CloudReviewers) can include both core and cloud engineers.

  1. Fix Issue: core team members submitting changes to the cloud repo were treated as external contributors because the internal check when getting reviewers was not checking the new team maps (CoreReviewers & CloudReviewers).
  2. Remove the Team field: since we’ve moved team assignments (core vs cloud vs docs) to distinct maps, this team field is not longer required and keeping it created ambiguity. It was previously used to determine which team (core vs cloud vs internal) a reviewer belonged to but team assignments are now determined by which map a reviewer is included in.
  3. Remove CodeReviewers map: I kept this old assignment map in the code in an attempt to keep the changes small when making the previous change to allow both core and cloud reviewers to review cloud changes but this turned out to be a bad decision because it introduced a bug where core engineers were considered external users when contributing to the cloud repo.
  4. Add PreferredOnly field to Reviewer struct: when true, a reviewer is only included in preferred reviewers and not the standard reviewer logic. This allows core reviewers in cloud to only be added as a reviewer for preferred reviewer paths.

Background:
This PR aligns the reviewer code with the reviewers.json data model. The data model has a number of related entities: teams, repositories, reviewers, and paths. Ideally, the top-level reviewer groups would be the repository since a preferredReviewerPath's context is a specific repository. Our data model adds a bit of ambiguity by defining teams (core or cloud), where the cloud team manages the cloud repository and the core team manages the teleport and teleport.e repositories. This change does not address the repository ambiguity but does allow for a single reviewer to be defined in both cloud and core teams.

@jimbishopp jimbishopp requested review from a team January 8, 2024 23:30
@jimbishopp jimbishopp force-pushed the jim/fix branch 3 times, most recently from dc72b4c to a8590ac Compare January 9, 2024 16:08
Comment on lines -388 to +390
v, ok := r.c.CodeReviewers[e.Author]
if !ok || v.Team == env.InternalTeam {
if !r.IsInternal(e.Author) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change resolves the issue where a core engineer not defined in the cloud reviewer map submits a PR to the cloud repo but requires admin approval. Calling IsInternal now checks core, docs, and cloud maps to determine if the author is internal.

@jimbishopp jimbishopp marked this pull request as draft January 29, 2024 16:10
@jimbishopp jimbishopp changed the title bot: Update internal check to include cloud and core maps bot: Fix cloud assignments from core engineers & refactor & add preferredonly flag Feb 7, 2024
@jimbishopp jimbishopp changed the title bot: Fix cloud assignments from core engineers & refactor & add preferredonly flag bot: Fix cloud reviewer assignments Feb 7, 2024
@jimbishopp jimbishopp marked this pull request as ready for review February 7, 2024 21:03
@jimbishopp jimbishopp merged commit 111ec9c into main Feb 8, 2024
8 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