fix(gitlab): enable skip-CI for merge requests#2405
Conversation
Summary of ChangesHello @theakshaypant, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves a critical bug in the GitLab integration that prevented the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🔍 PR Lint Feedback
|
There was a problem hiding this comment.
Code Review
This pull request aims to enable [skip ci] functionality for GitLab merge requests. The changes to include the "Merge Request" event type are correct. However, there's an issue in how the commit message is being parsed from the GitLab payload. It's currently using the commit title instead of the full commit message, which will prevent [skip ci] from working if the directive is in the commit body. I've added a specific comment with a suggested fix for this.
pkg/provider/gitlab/parse_payload.go
Outdated
| processedEvent.SHA = gitEvent.ObjectAttributes.LastCommit.ID | ||
| processedEvent.SHAURL = gitEvent.ObjectAttributes.LastCommit.URL | ||
| processedEvent.SHATitle = gitEvent.ObjectAttributes.LastCommit.Title | ||
| processedEvent.SHAMessage = gitEvent.ObjectAttributes.LastCommit.Title |
There was a problem hiding this comment.
The SHAMessage is being populated from LastCommit.Title, which only contains the subject line of the commit. To correctly detect [skip ci] directives, which may be in the commit body, you should use LastCommit.Message to get the full commit message. The GitLab merge request event payload includes the full message in this field.
| processedEvent.SHAMessage = gitEvent.ObjectAttributes.LastCommit.Title | |
| processedEvent.SHAMessage = gitEvent.ObjectAttributes.LastCommit.Message |
There was a problem hiding this comment.
if this is by design then test it explictely for it, and please explain with a comment why
There was a problem hiding this comment.
Ask❓
is that by design?
Answer:
No — populating SHAMessage from LastCommit.Title is very likely not “by design” for skip-CI detection; it’s more a pragmatic workaround to ensure SHAMessage is non-empty for GitLab merge request events.
In GitLab, LastCommit.Title is only the subject line. Skip directives like [skip ci] / [ci skip] are commonly placed in the full commit message (subject or body). If we only store the title, we can miss valid skip directives present in the body, which defeats the purpose of fixing skip-CI for MRs.
Given the PR description (“SHAMessage was not populated during payload parsing”), the intended design seems to be:
SHATitle= subject/titleSHAMessage= full commit message (subject + body) used byprovider.SkipCI(...)
So I’d recommend setting:
processedEvent.SHAMessage = gitEvent.ObjectAttributes.LastCommit.Message(falling back to Title only if Message is empty/unavailable), to align with the semantics of SHAMessage and make skip-CI detection robust.
There was a problem hiding this comment.
Code Implementation 🛠️
Implementation: Populate SHAMessage from the full GitLab commit message (with a safe fallback to the title) and add an inline comment explaining it is required for skip-CI detection which can be in the commit body.
| processedEvent.SHAMessage = gitEvent.ObjectAttributes.LastCommit.Title | |
| // Use the full commit message (not only the subject/title) so directives like `[skip ci]` | |
| // present in the commit body can be detected reliably. | |
| processedEvent.SHAMessage = strings.TrimSpace(gitEvent.ObjectAttributes.LastCommit.Message) | |
| if processedEvent.SHAMessage == "" { | |
| processedEvent.SHAMessage = gitEvent.ObjectAttributes.LastCommit.Title | |
| } |
See review comment here
17f63bd to
6ecaebd
Compare
|
/retest |
| // For PULL REQUEST events: commit message needs to be fetched via API | ||
| // Get commit info for skip-CI detection (only if we successfully set up client above) | ||
| if s.event.EventType == "pull_request" && repo != nil { | ||
| if (s.event.EventType == "pull_request" || s.event.EventType == "Merge Request") && repo != nil { |
There was a problem hiding this comment.
| if (s.event.EventType == "pull_request" || s.event.EventType == "Merge Request") && repo != nil { | |
| if (s.event.EventType == "pull_request" || s.event.EventType == "Merge_Request") && repo != nil { |
I think its Merge_Request
| processedEvent.SHA = gitEvent.ObjectAttributes.LastCommit.ID | ||
| processedEvent.SHAURL = gitEvent.ObjectAttributes.LastCommit.URL | ||
| processedEvent.SHATitle = gitEvent.ObjectAttributes.LastCommit.Title | ||
| processedEvent.SHAMessage = gitEvent.ObjectAttributes.LastCommit.Message |
There was a problem hiding this comment.
it may be both SHATitle + SHAMessage?
Skip-CI detection was not working for GitLab because the sinker only fetched commit info for pull_request events and SHAMessage was not populated during payload parsing. Jira: https://issues.redhat.com/browse/SRVKP-10440 Signed-off-by: Akshay Pant <akshay.akshaypant@gmail.com>
6ecaebd to
5a7a9b7
Compare
|
/review |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|



📝 Description of the Change
Skip-CI detection was not working for GitLab because the sinker only fetched commit info for pull_request events and SHAMessage was not populated during payload parsing.
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-10440
🔗 Linked GitHub Issue
N/A
🚀 Type of Change
fix:)feat:)feat!:,fix!:)docs:)chore:)refactor:)enhance:)deps:)🧪 Testing Strategy
🤖 AI Assistance
If you have used AI assistance, please provide the following details:
Which LLM was used?
Extent of AI Assistance:
Important
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Gemini gemini@google.com
Co-authored-by: ChatGPT noreply@chatgpt.com
Co-authored-by: Claude noreply@anthropic.com
Co-authored-by: Cursor noreply@cursor.com
Co-authored-by: Copilot Copilot@users.noreply.github.com
**💡You can use the script
./hack/add-llm-coauthor.shto automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.