Repository navigation
feat(api):add support for creating/editing reviewers in project MRs - #1396
nejch merged 2 commits into
Conversation
Sorry, something went wrong.
Codecov Report
@@ Coverage Diff @@
## master #1396 +/- ##
==========================================
- Coverage 90.98% 82.29% -8.69%
==========================================
Files 73 74 +1
Lines 4080 4162 +82
==========================================
- Hits 3712 3425 -287
- Misses 368 737 +369
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Sorry, something went wrong.
JohnVillalovos
left a comment
There was a problem hiding this comment.
Thanks a lot for this! Sorry for the delay in getting a review done.
I have left some comments.
I also rebased the patch to bring it in sync with the upstream branch.
Sorry, something went wrong.
| "remove_source_branch", | ||
| "allow_maintainer_to_push", | ||
| "squash", | ||
| "reviewer_ids", |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
nejch
left a comment
There was a problem hiding this comment.
Sorry, somehow I missed this!
Since we reduced the scope I think we can go ahead with this PR and I'll open a follow-up/upstream issue to see if we can align the parameters. The OpenAPI topic is already discussed here #1446 and we can hopefully come up with a general solution that covers everything there.
Sorry, something went wrong.
|
Hello! I just read through the changes and discussions in this MR, and would be interested in following the upstream issue(s) with GitLab regarding aligning the parameters. @nejch apologies if I missed a link somewhere in the discussions, but could you please share the GitLab upstream issue? Appreciate it! |
Sorry, something went wrong.
This is a first attempt at allowing users to create MRs with reviewers or edit an existing MR with the reviewer_ids parameter.
This is kind of unique in the sense that reviewer_ids doesn't appear in the rest response, but it is an accepted parameter.
A manager might be a better way to implement this, but I'm not sure it's appropriate since reviewers doesn't have its own API path, so reviewers are not really a rest object in their own right. The only example of a manager being used as a helper is the sidekiq manager.
API:
The property can be omitted and the same code as above works, however mr.reviewer_ids would be an attribute error, which may be an odd interface.
What I have here works but I'm not sure it's an ideal interface. Mainly raising this to get feedback from maintainers on how this should be implemented. Feedback appreciated.
Relates to #1267