Skip to content

feat(api):add support for creating/editing reviewers in project MRs #1396

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 26, 2021
Merged

feat(api):add support for creating/editing reviewers in project MRs #1396

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
676d1f6
feat(api): add support for creating/editing reviewers in project merg…
spyoungtech Apr 14, 2021
3d985ee
feat(api): remove responsibility for API inconsistencies for MR revie…
spyoungtech Jun 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions gitlab/v4/objects/merge_requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,7 @@ class ProjectMergeRequestManager(CRUDMixin, RESTManager):
"remove_source_branch",
"allow_maintainer_to_push",
"squash",
"reviewer_ids",
Copy link
Member

@JohnVillalovos JohnVillalovos May 31, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As long as we are updating this I think it would be good to bring it up to date with the current API

https://docs.gitlab.com/ee/api/merge_requests.html#create-mr

These seem missing to me:

  • assignee_ids | integer array | no | The ID of the user(s) to assign the MR to. Set to 0 or provide an empty value to unassign all assignees.
  • reviewer_ids | integer array | no | The ID of the user(s) added as a reviewer to the MR. If set to 0 or left empty, no reviewers are added.
  • remove_source_branch | boolean | no | Flag indicating if a merge request should remove the source branch when merging.

Also it is nice if they are in the order shown in the API docs. As makes it easier to try to figure out what we are missing.

Copy link
Contributor Author

@spyoungtech spyoungtech Jun 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for taking a look at this.

Yeah, what's interesting about this particular API is that, unlike most other endpoints, the POST body includes parameters that are not included in the response.

One consequence I noticed is that you'd get a (arguably unexpected) error when trying to access the reviewer_ids attribute because it doesn't really exist in the response.

This is not a huge problem, but was a motivation behind the @property getter/setter for reviewer_ids in an attempt to keep the Python API more consistent.

If the idea is to be able to do something like this:

mr.reviewer_ids = [1,2,3]
mr.save()

Then mr.reviewer_ids should always be an accessible attribute even on a freshly retrieved object, in my opinion. Without the property, this will be an attribute error.

If this is seen as a good approach, I'd be happy to add the same for assignees (it has the same oddities, IIRC).

I'll also be happy to take care of the chore of adding remove_source_branch and double-checking for any other missing parameters for MRs.

),
)
_update_attrs = RequiredOptional(
Expand All @@ -388,6 +389,7 @@ class ProjectMergeRequestManager(CRUDMixin, RESTManager):
"discussion_locked",
"allow_maintainer_to_push",
"squash",
"reviewer_ids",
),
)
_list_filters = (
Expand Down