Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow RemoveReviewers to remove only teams #3337

Merged
merged 3 commits into from
Oct 27, 2024

Conversation

nagl-resourcely
Copy link
Contributor

@nagl-resourcely nagl-resourcely commented Oct 26, 2024

What?

Fixes a JSON marshaling problem that causes spurious 422s when calling RemoveReviewrs with TeamReviewers but no Reviewers.

Background

The github API requires "reviewers". It permits "reviewers": [] but not "reviewers": null. Try it:

echo "Show the problem"
curl -L \                                                            
  -X DELETE \   
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer ${PAT}" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/repos/${OWNER}/${REPO}/pulls/${NUMBER}/requested_reviewers \
  -d '{"team_reviewers":["the_senate"]}' 
{
  "message": "Invalid request.\n\n\"reviewers\" wasn't supplied.",
  "documentation_url": "https://docs.github.com/rest/pulls/review-requests#remove-requested-reviewers-from-a-pull-request",
  "status": "422"
}

echo "What if we just removed omitempty?"
curl -L \
  -X DELETE \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer ${PAT}" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/repos/${OWNER}/${REPO}/pulls/${NUMBER}/requested_reviewers \
  -d '{"reviewers": null, "team_reviewers":["the_senate"]}'
{
  "message": "Invalid request.\n\nFor 'properties/reviewers', nil is not an array.",
  "documentation_url": "https://docs.github.com/rest/pulls/review-requests#remove-requested-reviewers-from-a-pull-request",
  "status": "422"
}

echo "Does the empty list work?"
curl -L \
  -X DELETE \
  -H "Accept: application/vnd.github+json" \
  -H "Authorization: Bearer ${PAT}" \
  -H "X-GitHub-Api-Version: 2022-11-28" \
  https://api.github.com/repos/${OWNER}/${REPO}/pulls/${NUMBER}/requested_reviewers \
  -d '{"reviewers": [], "team_reviewers":["the_senate"]}'
{
  "url": ...<spam>,

Testing?

There's a new unit test. Without the 2nd commit, it fails with:

    pulls_reviewers_test.go:181: request Body is {"team_reviewers":["justice-league"]}
        , want {"reviewers":[],"team_reviewers":["justice-league"]}

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @nagl-resourcely!
One nit, otherwise LGTM.

Then we will be ready for a second LGTM+Approval from any other contributor to this repo before merging.

github/pulls_reviewers.go Outdated Show resolved Hide resolved
@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Oct 26, 2024
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
@nagl-resourcely
Copy link
Contributor Author

Ty for expressing the nit as a diff =)

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @nagl-resourcely !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

Copy link

codecov bot commented Oct 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.02%. Comparing base (2b8c7fa) to head (651c746).
Report is 163 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3337      +/-   ##
==========================================
- Coverage   97.72%   93.02%   -4.71%     
==========================================
  Files         153      172      +19     
  Lines       13390    14860    +1470     
==========================================
+ Hits        13085    13823     +738     
- Misses        215      944     +729     
- Partials       90       93       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@tomfeigin tomfeigin left a comment

Choose a reason for hiding this comment

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

LGTM

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Oct 27, 2024
@gmlewis
Copy link
Collaborator

gmlewis commented Oct 27, 2024

Thank you, @tomfeigin !
Merging.

@gmlewis gmlewis merged commit 9e5757d into google:master Oct 27, 2024
6 of 7 checks passed
@nagl-resourcely nagl-resourcely deleted the allow-removing-only-teams branch October 28, 2024 15:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants