iframe-proxy

isaac-philip · 2026-03-29T17:25:21Z

Status of Merge Request on Merge Train
Add Merge Request to Merge Train

codecov · 2026-03-29T17:44:51Z

isaac-philip · 2026-03-29T19:09:24Z

@JohnVillalovos kindly have a look for same set of changes with earlier MR rebased on latest. thanks.

Copilot

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 ProjectMergeTrainManager to support get() and add a nested merge_requests manager 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.

isaac-philip · 2026-04-11T18:44:43Z

@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.

JohnVillalovos · 2026-04-13T15:09:37Z

@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.

JohnVillalovos · 2026-04-13T15:10:34Z

As a note. It would be nice if it was squashed down to maybe 1 commit? Less than 12 sounds like a good start.

JohnVillalovos · 2026-04-18T17:12:19Z

@isaac-philip Now it is 13 commits. Let me know if you need help squashing

isaac-philip · 2026-04-19T13:17:28Z

Hello @JohnVillalovos , have made the change as requested for single commit, thanks.

Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>

Copilot

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.

Copilot · 2026-04-19T18:01:43Z

+        if hasattr(parent_ref, "iid"):
+            return cast(int, parent_ref.iid)


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.

Suggested change

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

Copilot · 2026-04-19T18:01:44Z

+                _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 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.

Suggested change

_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.

Copilot · 2026-04-19T18:01:44Z

+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"]
+    )


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.

JohnVillalovos · 2026-04-19T18:35:08Z

@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.

JohnVillalovos · 2026-04-19T18:50:06Z

        self._parent_attrs = data
        return path.format(**data)

+    def _get_parent_ref_id(self) -> int | str | None:


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.

JohnVillalovos · 2026-04-19T19:08:08Z

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 ?

JohnVillalovos requested a review from Copilot March 29, 2026 20:13

Copilot started reviewing on behalf of JohnVillalovos March 29, 2026 20:13 View session

Copilot AI reviewed Mar 29, 2026

View reviewed changes

Comment thread gitlab/v4/objects/projects.py Outdated

Comment thread tests/unit/objects/test_merge_trains.py Outdated

Comment thread tests/unit/objects/test_merge_trains.py

Comment thread gitlab/v4/objects/merge_trains.py

isaac-philip force-pushed the feat/merge_trains_additional_update branch from b2e70cc to f1b8827 Compare April 19, 2026 12:57

feat(functional): merge-train api add and status python-gitlab#2547

64e017d

Signed-off-by: Isaac Philip <4974658+isaac-philip@users.noreply.github.com>

isaac-philip force-pushed the feat/merge_trains_additional_update branch from f1b8827 to 64e017d Compare April 19, 2026 16:26

JohnVillalovos requested a review from Copilot April 19, 2026 17:55

Copilot started reviewing on behalf of JohnVillalovos April 19, 2026 17:56 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

JohnVillalovos requested changes Apr 19, 2026

View reviewed changes

		if hasattr(parent_ref, "iid"):
		return cast(int, parent_ref.iid)

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

isaac-philip commented Mar 29, 2026

Uh oh!

codecov Bot commented Mar 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

isaac-philip commented Mar 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

isaac-philip commented Apr 11, 2026

Uh oh!

JohnVillalovos commented Apr 13, 2026

Uh oh!

JohnVillalovos commented Apr 13, 2026

Uh oh!

JohnVillalovos commented Apr 18, 2026

Uh oh!

isaac-philip commented Apr 19, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

JohnVillalovos commented Apr 19, 2026

Uh oh!

JohnVillalovos Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

JohnVillalovos commented Apr 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Mar 29, 2026 •

edited

Loading