Feat/merge trains additional #2547#3381
Feat/merge trains additional #2547#3381isaac-philip wants to merge 8 commits intopython-gitlab:mainfrom
Conversation
…dd mr Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>
…dd mr - test data modifications Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>
…dd mr - minor changes Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>
…dd mr - lint formatted by black Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>
…dd mr - docs added Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3381 +/- ##
==========================================
+ Coverage 92.16% 95.76% +3.59%
==========================================
Files 100 100
Lines 6125 6133 +8
==========================================
+ Hits 5645 5873 +228
+ Misses 480 260 -220
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
@JohnVillalovos kindly have a look for same set of changes with earlier MR rebased on latest. thanks. |
There was a problem hiding this comment.
Pull request overview
Adds support for GitLab’s merge train merge-request endpoints (MR status on merge train + adding an MR to a merge train) to python-gitlab, along with unit tests and docs, addressing issue #2547.
Changes:
- Extend
ProjectMergeTrainManagerto supportget()and add a nestedmerge_requestsmanager for merge train MR status/add-to-train endpoints. - Add unit tests covering merge train listing, MR status retrieval, and adding an MR to a merge train.
- Document the new merge train MR classes/managers and provide usage examples.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
gitlab/v4/objects/merge_trains.py |
Adds merge train MR REST object + manager; enables get() on merge trains. |
gitlab/v4/objects/projects.py |
Adjusts import for merge trains manager (lint-sensitive). |
tests/unit/objects/test_merge_trains.py |
Adds fixtures and tests for new merge train MR endpoints. |
docs/gl_objects/merge_trains.rst |
Documents new merge train MR objects/managers and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) | ||
| from .merge_requests import ProjectMergeRequestManager # noqa: F401 | ||
| from .merge_trains import ProjectMergeTrainManager # noqa: F401 | ||
| from .merge_trains import ProjectMergeTrainManager |
There was a problem hiding this comment.
ProjectMergeTrainManager is imported but only referenced via annotations/manager auto-creation, which flake8/pyflakes still treats as an unused import in this file (many similar imports here use # noqa: F401). Removing # noqa: F401 is likely to trigger F401 in the lint job; please restore the ignore (or otherwise reference the name at runtime).
| from .merge_trains import ProjectMergeTrainManager | |
| from .merge_trains import ProjectMergeTrainManager # noqa: F401 |
| } | ||
|
|
||
| merge_train_update = deepcopy(mr_content) | ||
| merge_train_update["iid"] = 4 |
There was a problem hiding this comment.
The test data for merge_train_update sets a top-level iid key, but mr_content models iid under the nested merge_request object. This makes the mocked POST response inconsistent with the schema used elsewhere in the test and can hide mistakes in assertions. Update the fixture so the merge request IID change is applied to merge_train_update["merge_request"]["iid"] (and/or adjust other fields) rather than adding a new top-level key.
| merge_train_update["iid"] = 4 | |
| merge_train_update["merge_request"]["iid"] = 4 |
| url="http://localhost/api/v4/projects/1/merge_trains/merge_requests/4", | ||
| json=[merge_train_update], | ||
| content_type="application/json", | ||
| status=200, |
There was a problem hiding this comment.
The POST fixture for merge_trains/merge_requests/:iid doesn't assert the request body, so the test can pass even if update() sends the wrong payload (or no payload). Consider adding a responses.matchers.json_params_matcher (or equivalent) to validate that the expected fields (e.g., sha) are included in the POST body.
| status=200, | |
| status=200, | |
| match=[ | |
| responses.matchers.json_params_matcher({"sha": "ef33a3zxc3"}), | |
| ], |
| class ProjectMergeTrainMergeRequest(RESTObject): | ||
| pass | ||
|
|
||
|
|
||
| class ProjectMergeTrainManager(ListMixin[ProjectMergeTrain]): | ||
| class ProjectMergeTrainMergeRequestManager( | ||
| GetMixin[ProjectMergeTrainMergeRequest], | ||
| UpdateMixin[ProjectMergeTrainMergeRequest], | ||
| RESTManager[ProjectMergeTrainMergeRequest], | ||
| ): | ||
| _path = "/projects/{project_id}/merge_trains/merge_requests" | ||
| _obj_cls = ProjectMergeTrainMergeRequest | ||
| _from_parent_attrs = {"project_id": "project_id"} | ||
| _update_method: UpdateMethod = UpdateMethod.POST |
There was a problem hiding this comment.
ProjectMergeTrainMergeRequestManager.get(<id>) and .update(<id>) use the merge request IID in the URL, but ProjectMergeTrainMergeRequest currently uses the default _id_attr = "id" (the merge-train entry id returned by the API). That makes merge_train_mr.get_id() return a different identifier than what the manager methods expect, which is error-prone for users. Consider overriding _id_attr/get_id() so the object identifier aligns with the merge request IID used by these endpoints (or otherwise make the distinction explicit).
closes #2547