-
Notifications
You must be signed in to change notification settings - Fork 117
fix: add explicit mapping for cancelled status #2291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: add explicit mapping for cancelled status #2291
Conversation
- Add case for 'cancelled' conclusion in GitLab provider CreateStatus - Map 'cancelled' to 'canceled' for GitLab API compatibility - Set descriptive title 'cancelled validating this commit' - Ensures proper status reporting when PipelineRuns are cancelled via PR close
Summary of ChangesHello @ab-ghosh, 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 an issue where the GitLab provider did not correctly interpret the 'cancelled' status. It introduces a specific case to map 'cancelled' to 'canceled' for GitLab API compatibility and sets an appropriate status title. A corresponding unit test has been added to ensure the new handling works as expected. 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.
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 correctly adds support for the 'cancelled' status in the GitLab provider by mapping it to 'canceled' for API compatibility, and includes a unit test to verify the new logic. The implementation is sound. I have one suggestion to refactor the code slightly by combining similar cases, which would improve code clarity and maintainability.
case "neutral": | ||
statusOpts.Conclusion = "canceled" | ||
statusOpts.Title = "stopped" | ||
case "cancelled": | ||
statusOpts.Conclusion = "canceled" | ||
statusOpts.Title = "cancelled validating this commit" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for neutral
and cancelled
conclusions is very similar. You can combine these cases to reduce code duplication and improve maintainability.
case "neutral": | |
statusOpts.Conclusion = "canceled" | |
statusOpts.Title = "stopped" | |
case "cancelled": | |
statusOpts.Conclusion = "canceled" | |
statusOpts.Title = "cancelled validating this commit" | |
case "neutral", "cancelled": | |
if statusOpts.Conclusion == "neutral" { | |
statusOpts.Title = "stopped" | |
} else { | |
statusOpts.Title = "cancelled validating this commit" | |
} | |
statusOpts.Conclusion = "canceled" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather duplication than code spagetthi, so you can discard this
can we fix canceled across the codebase ? |
I've reviewed the codebase and didn't find any other inconsistencies related to the use of “canceled” for GitLab. |
@chmouel what do you mean by fixing? removing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feel free to resolve the comment if it is incorrect, added the comment more for learning for myself.
📝 Description of the Change
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-9050
🔗 Linked GitHub Issue
Fixes #
🚀 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-by
trailer to your commit message.For example:
Co-authored-by: Gemini [email protected]
Co-authored-by: ChatGPT [email protected]
Co-authored-by: Claude [email protected]
Co-authored-by: Cursor [email protected]
Co-authored-by: Copilot [email protected]
**💡You can use the script
./hack/add-llm-coauthor.sh
to automatically addthese co-author trailers to your commits.
✅ Submitter Checklist
fix:
,feat:
) matches the "Type of Change" I selected above.make test
andmake lint
locally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit install
toautomate these checks.