Feat/merge trains additional #2547#3381
Feat/merge trains additional #2547#3381isaac-philip wants to merge 1 commit intopython-gitlab:mainfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3381 +/- ##
==========================================
+ Coverage 92.16% 95.77% +3.61%
==========================================
Files 100 100
Lines 6125 6152 +27
==========================================
+ Hits 5645 5892 +247
+ 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.
|
@JohnVillalovos appreciate the review, kindly check if it looks good now after review changes. any more suggestions also I would be happy to rectify sooner. thank you. |
Thanks. I will try to find time and motivation to review this. Hopefully this week. |
|
As a note. It would be nice if it was squashed down to maybe 1 commit? Less than 12 sounds like a good start. |
|
@isaac-philip Now it is 13 commits. Let me know if you need help squashing |
b2e70cc to
f1b8827
Compare
|
Hello @JohnVillalovos , have made the change as requested for single commit, thanks. |
Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>
f1b8827 to
64e017d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if hasattr(parent_ref, "iid"): | ||
| return cast(int, parent_ref.iid) |
There was a problem hiding this comment.
RESTManager._get_parent_ref_id() only supports parent refs that expose an .iid attribute. In this codebase many nested refs (including merge train merge_request) are plain dicts, so this fallback will silently return None even when the dict contains an iid key. Consider supporting dict-like refs (e.g., isinstance(parent_ref, dict) and 'iid' in parent_ref) and returning the raw value without casting to int so str IIDs are also handled correctly.
| if hasattr(parent_ref, "iid"): | |
| return cast(int, parent_ref.iid) | |
| if isinstance(parent_ref, dict) and "iid" in parent_ref: | |
| return parent_ref["iid"] | |
| if hasattr(parent_ref, "iid"): | |
| return parent_ref.iid |
| _parent_ref_id from the parent object (if available). | ||
| lazy: If True, don't request the server, but create a | ||
| shallow object giving access to the managers. This is | ||
| useful if you want to avoid useless calls to the API. |
There was a problem hiding this comment.
The GetMixin.get() docstring mentions falling back to _parent_ref_id, but the implementation uses self._get_parent_ref_id() and relies on _parent_ref_attr. Update the wording so it matches the actual mechanism (and attribute name) to avoid confusing API consumers.
| _parent_ref_id from the parent object (if available). | |
| lazy: If True, don't request the server, but create a | |
| shallow object giving access to the managers. This is | |
| useful if you want to avoid useless calls to the API. | |
| the parent reference resolved by ``self._get_parent_ref_id()`` | |
| when available (for example, when ``_parent_ref_attr`` is set | |
| on the manager). | |
| lazy: If True, don't request the server, but create a shallow | |
| object giving access to the managers. This is useful if you | |
| want to avoid useless calls to the API. |
| def test_merge_train_add_merge_request(project, resp_merge_trains_merge_request_post): | ||
| merge_train: ProjectMergeTrain = project.merge_trains.get(1, lazy=True) | ||
| merge_requests_update = merge_train.merge_requests.update( | ||
| 4, new_data={"sha": "ef33a3zxc3"} | ||
| ) | ||
| assert isinstance(merge_train, ProjectMergeTrain) | ||
| assert ( | ||
| merge_requests_update[0]["pipeline"]["sha"] | ||
| == merge_train_update["pipeline"]["sha"] | ||
| ) | ||
| assert ( | ||
| merge_requests_update[0]["merge_request"]["iid"] | ||
| == merge_train_update["merge_request"]["iid"] | ||
| ) |
There was a problem hiding this comment.
This test treats merge_train.merge_requests.update(...) as returning a list (indexing [0]), but UpdateMixin.update() is typed/documented as returning a dict. Please align the expected response shape with the actual GitLab API response and/or wrap/override the manager method with a correctly typed return value so users don't get misleading type hints.
|
@isaac-philip Please make the commit message a bit more informative Currently it is basically just a subject line that says:
It is hard to tell from the message what is being added, what “status” refers to, or whether this is about merge train objects, merge train merge requests, or functional test coverage. Also the PR number should not be in the commit message. |
| self._parent_attrs = data | ||
| return path.format(**data) | ||
|
|
||
| def _get_parent_ref_id(self) -> int | str | None: |
There was a problem hiding this comment.
Is this really needed? Is it really used? I'd prefer to not add this if it isn't needed. It doesn't seem like the tests (non-mixin ones) actually end up using this.
Or I don't understand the changes very well.
|
I was reviewing this with the help of Claude Code. I ended up coming up with this. The nesting of # ProjectDeploymentMergeRequest — deployment id IS in the path:
_path = "/projects/{project_id}/deployments/{deployment_id}/merge_requests"
_from_parent_attrs = {"deployment_id": "id", "project_id": "project_id"}
# ProjectMergeRequestDiffVersion — MR iid IS in the path:
_path = "/projects/{project_id}/merge_requests/{mr_iid}/versions"
_from_parent_attrs = {"project_id": "project_id", "mr_iid": "iid"}In both cases project.merge_trains.get(1, lazy=True).merge_requests.get(5) # GET /projects/1/merge_trains/merge_requests/5
project.merge_trains.get(999, lazy=True).merge_requests.get(5) # GET /projects/1/merge_trains/merge_requests/5 (identical)The The GitLab API endpoint # projects.py
merge_trains: ProjectMergeTrainManager
merge_train_merge_requests: ProjectMergeTrainMergeRequestManager
# merge_trains.py
class ProjectMergeTrainMergeRequestManager(...):
_path = "/projects/{project_id}/merge_trains/merge_requests"
_from_parent_attrs = {"project_id": "id"} # "id" from Project directlyUsage becomes What do you think @isaac-philip ? |
closes #2547