Skip to content

Add target::zstream label to MRs at creation time - #662

Merged
lbarcziova merged 2 commits into
packit:mainfrom
lbarcziova:zstream-target-label
Jul 9, 2026
Merged

lbarcziova merged 2 commits into
packit:mainfrom
lbarcziova:zstream-target-label

Conversation

@lbarcziova

@lbarcziova lbarcziova commented Jul 8, 2026 •

Copy link
Copy Markdown
Member

Fixes: https://redhat.atlassian.net/browse/PACKIT-5088

Assisted-by: Claude Opus 4.6

TODO:

  • test the MR creation

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces atomic labeling for GitLab merge requests at creation time to prevent webhook race conditions, specifically targeting the target::zstream label across various agents (backport, consolidation, rebase, and rebuild). Feedback on these changes highlights a critical bug in OpenMergeRequestTool._create_mr where the response_code and message are not propagated to GitlabAPIException, breaking the 409 Conflict handling for existing merge requests. Additionally, a minor improvement is suggested in mr_consolidation_agent.py to safely handle potentially None label values and avoid a TypeError.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +365 to +368
try:
raw_mr = project.gitlab_repo.mergerequests.create(parameters)
except gitlab.GitlabError as ex:
raise GitlabAPIException() from ex

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

In _create_mr, when catching gitlab.GitlabError, a new GitlabAPIException is raised without passing the original exception's response_code or message.

This causes ex.response_code to be None when handled in _run (line 390), which prevents the 409 Conflict check (if ex.response_code == 409:) from succeeding. As a result, the tool will fail with an unhandled exception instead of gracefully reusing the existing merge request.

Please propagate the response_code and message from the caught GitlabError to the GitlabAPIException.

Suggested change
try:
raw_mr = project.gitlab_repo.mergerequests.create(parameters)
except gitlab.GitlabError as ex:
raise GitlabAPIException() from ex
try:
raw_mr = project.gitlab_repo.mergerequests.create(parameters)
except gitlab.GitlabError as ex:
raise GitlabAPIException(message=str(ex), response_code=ex.response_code) from ex

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitlabAPIException.response_code is a property that reads from self.cause, which is set by the from ex clause. The 409 should work correctly through the exception chain

Comment thread ymir/agents/mr_consolidation_agent.py Outdated
Include the label atomically in the GitLab API create call to avoid
a webhook race condition where CI picks up the MR before post-creation
labels arrive. Uses python-gitlab directly instead of OGR for all MR
creation (consistent code path regardless of labels).

Fixes: https://redhat.atlassian.net/browse/PACKIT-5088

Assisted-by: Claude Opus 4.6
@lbarcziova
lbarcziova force-pushed the zstream-target-label branch from 66fd14e to 49243ea Compare July 9, 2026 11:05
@nforro
nforro self-requested a review July 9, 2026 12:35
Comment thread ymir/agents/mr_consolidation_agent.py Outdated
available_tools=gateway_tools,
)

source_labels = {label for mr in state.all_open_mrs for label in (mr.get("labels") or [])}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only two oldest MRs are being consolidated, source_labels should come only from them.

Comment on lines -996 to -1006

if state.merge_request_url:
try:
await run_tool(
"add_merge_request_labels",
merge_request_url=state.merge_request_url,
labels=["ymir_backport"],
available_tools=gateway_tools,
)
except Exception as e:
logger.warning("Failed to label consolidated MR: %s", e)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From Claude:
The old post-creation add_merge_request_labels call is removed here, but open_merge_request doesn't update labels on the 409 (existing MR) path. The other agents are fine because tasks.commit_push_and_open_mr still has its own add_merge_request_labels fallback, but the consolidation agent calls open_merge_request directly.

Scope source labels to the two selected MRs (via state.mr_urls),
add label fallback for 409-reused MRs, and fix is_new → is_new_mr
key to match OpenMergeRequestResult.

Assisted-by: Claude Opus 4.6
@lbarcziova

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@lbarcziova

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

@nforro nforro left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lbarcziova
lbarcziova merged commit 6f9d28a into packit:main Jul 9, 2026
11 checks passed
@lbarcziova
lbarcziova deleted the zstream-target-label branch July 9, 2026 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants