Codecov Report✅ All modified and coverable lines are covered by tests. @@ 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.
... and 22 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
|
@JohnVillalovos kindly have a look for same set of changes with earlier MR rebased on latest. thanks. |
Sorry, something went wrong.
There was a problem hiding this comment.
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:
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| 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.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
As a note. It would be nice if it was squashed down to maybe 1 commit? Less than 12 sounds like a good start. |
Sorry, something went wrong.
|
@isaac-philip Now it is 13 commits. Let me know if you need help squashing |
Sorry, something went wrong.
|
Hello @JohnVillalovos , have made the change as requested for single commit, thanks. |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| 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 |
Sorry, something went wrong.
| _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. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
@isaac-philip Please make the commit message a bit more informative Currently it is basically just a subject line that says: feat(functional): merge-train api add and status #2547 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. |
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
|
I was reviewing this with the help of Claude Code. I ended up coming up with this. The nesting of ProjectMergeTrainMergeRequestManager under ProjectMergeTrain is misleading because the merge train's ID is never used in the URL. Compare with the established pattern in this codebase: # 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 _from_parent_attrs includes the parent's own ID so it genuinely scopes the child resource. Here, _from_parent_attrs = {"project_id": "project_id"} only borrows project_id — the merge train's id is silently discarded. This means: 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 1 vs 999 makes no difference. A user would reasonably expect the merge train ID to matter. The GitLab API endpoint /projects/:id/merge_trains/merge_requests/:iid is a project-level resource, not scoped to a specific merge train. The consistent approach would be to attach the manager directly to Project, alongside merge_trains: # 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 directly
Usage becomes project.merge_train_merge_requests.get(mr_iid) — accurate and consistent with the rest of the codebase. What do you think @isaac-philip ? |
Sorry, something went wrong.
closes #2547