Add reviewers without replacing the reviewer list

What does this MR do and why?

Bot writes that add a reviewer read the reviewer list, add to it and PUT the whole list back, because REST cannot add one reviewer. When two land a second apart, the second drops the reviewer the first added. #1889 (closed) found it on 21 merge requests in gitlab-org/gitlab in a week. On 26 October it hit gitlab-org/gitlab!265240: the ready command requested a reviewer, and 7 seconds later AutomatedReviewRequestDoc requested a technical writer and removed that reviewer.

This MR adds reviewers with the mergeRequestSetReviewers mutation in APPEND mode, which keeps the reviewers the merge request has when the write lands.

  • Reaction#assign_reviewers_by_username sends it through a new ApiStrategy::GraphQL#add_reviewers. The mutation takes usernames, so the user lookups go. Its callers: AutomatedReviewRequestDoc, CommandMrHelp, GitlabUiUpdateReviewerAssignment.
  • CommandMrRequestReview#request_reviews adds new reviewers the same way and still re-requests the current ones.
  • Reaction#assign_reviewers goes: it merged the event's stale reviewer_ids and had no other caller. set_reviewers stays for unassign_review.

#1889 (closed)'s other three fixes are in !4482 (merged). Its reordering of the ready command would have prevented the case above on its own; this one covers any two bot writes that add reviewers. The two merge with no conflict in either order. !4482 (merged) also removes a timing flake in a rate limit example, one that master has too (below).

Related to #1889 (closed)

Expected impact & dry-runs

No policy changes, so no policy dry-run applies. I have not run it against the playground project.

Before After
Two bot writes add a reviewer a second apart The second drops the first one's reviewer Both stay
A username GitLab cannot resolve Dropped silently by the lookup Printed from the mutation's errors, the others added
A reviewer who cannot read the merge request Dropped from the PUT GitLab refuses the write, and its reason is printed

A refused mutation is printed rather than raised, as for rerequest_review, so the ready command still posts its comment. It needs the same permissions as the PUT, and validates against GitLab.com's GraphQL schema (19.6).

Checked in ruby:4.0.9 on master at 071df853: the changed specs give 163 examples, 63 failures on master's code, and 0 failures with this change in all 5 runs.

The other checks
  • Suite: 7446 examples, 0 failures in all 4 runs (master: 7425 examples, 0 failures in all 3).
  • rubocop -P -E .: 682 files, no offenses.
  • Coverage: 98.36%, and undercover -c against master reports nothing missing.
  • Merged with !4482 (merged): 7472 examples, 0 failures, in both runs.
  • Danger's commit and title linters: no problems.
The timing example

rate limited API requests for a team member, in command_mr_request_review_spec.rb, sends 100 commands that must finish within about a second, because RateLimit stores its counter again on each one with the time left rounded to whole seconds. Earlier today, on the previous master and with other suites loading this machine, it failed in some whole-suite runs of master and of this branch. At lower load it passed 8 of 8 runs alone on each: 0.5 to 0.8 s on this branch, 0.4 to 0.8 s on master. !4482 (merged) keeps the expiry the first command set, so the time stops mattering: on the merged tree it took 1.0 to 1.4 s and passed all 8.

Failures on master's code, per spec file
      4 spec/processor/community/automated_review_request_doc_spec.rb
     50 spec/processor/community/command_mr_request_review_spec.rb
      1 spec/processor/gitlab_ui_update_reviewer_assignment_spec.rb
      4 spec/triage/api_strategy/graph_ql_spec.rb
      4 spec/triage/reaction_spec.rb

Action items

  • No environment variable is added.
  • unassign_review still writes the whole list. operationMode: REMOVE would fix that, here or in a follow-up, as you prefer.

Merge request reports

Loading
Loading