-
Notifications
You must be signed in to change notification settings - Fork 116
fix: prevent stale failure statuses on commits #2288
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: prevent stale failure statuses on commits #2288
Conversation
Summary of ChangesHello @zakisk, 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 problem where GitLab commit statuses would incorrectly remain marked as 'failed' due to inconsistent context keys. Previously, initial validation failures used a generic context key, while subsequent successful runs used a more specific one, preventing the successful status from overwriting the original failure. The change introduces the use of the 'OriginalPRName' annotation to provide a uniform PipelineRun name for the context key, allowing successful PipelineRuns to correctly update the commit status and reflect the true state of the pipeline. 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
|
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 addresses an issue where stale failure statuses would persist on GitLab commits due to inconsistent context keys. The change ensures that when a PipelineRun fails to be created (e.g., due to a validation error), the failure status is created with a consistent context key derived from the OriginalPRName
annotation. This allows subsequent successful runs to correctly overwrite the status. The implementation is sound and correctly solves the described problem. I have no further comments.
077d55f
to
e325263
Compare
} | ||
|
||
func (o *OpenshiftConsole) URL() string { | ||
if o.host == "" { |
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.
if PaC is not set with any Console, it was also causing GitLab API an error "{target_url: [is blocked: URI is invalid]}}"
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.
Please instead add a func (o *OpenshiftConsole) Host()
getter which defaults to openshift.url.is.not.configured
when o.host
is not set, so we're not duplicating this fallback logic in every function
/test |
🔍 PR Lint Feedback
|
can we have e2e test please |
I thought about it but there is no way to confirm that how job are posted because GitLab doesn't provide API endpoint to check jobs in a commit status pipeline |
are you sure? that surprises me, gemini says otherwise as well:
|
yeah, I was kinda obsessed with my previous findings and missed this. we can do it using commit status api |
Ensures a consistent context key is used for all GitLab commit statuses, allowing failures from invalid PipelineRuns to be correctly overwritten on subsequent runs. The Problem: When a repository contained multiple PipelineRun definitions and one was invalid (e.g., due to a validation error), the initial commit would correctly report a "failed" status to GitLab for that invalid run. However, the context key for this failure was generic (e.g., "ApplicationName") because a full PipelineRun wasn't provided. After a developer fixed the invalid PipelineRun and pushed a new commit, the now-successful run would post its status with a more specific context key (e.g., "ApplicationName/PipelineRunName"). Because the GitLab API does not allow deleting commit pipeline jobs, the original, generic "failed" status remained on the commit forever. This resulted in the overall commit pipeline being permanently and incorrectly marked as "failed," even though all pipelines eventually succeeded. The Solution: this commit provides PipelineRun name from "OriginalPRName" annotation so that contextKey is uniform for "success" PipelineRun and failed PipelineRuns due to validation. https://issues.redhat.com/browse/SRVKP-9044 Signed-off-by: Zaki Shaikh <[email protected]>
e325263
to
a77f07f
Compare
@chmouel added E2E test |
Ensures a consistent context key is used for all GitLab commit statuses, allowing failures from invalid PipelineRuns to be correctly overwritten on subsequent runs.
The Problem:
When a repository contained multiple PipelineRun definitions and one was invalid (e.g., due to a validation error), the initial commit would correctly report a "failed" status to GitLab for that invalid run. However, the context key for this failure was generic (e.g., "ApplicationName") because a full PipelineRun wasn't provided.
After a developer fixed the invalid PipelineRun and pushed a new commit, the now-successful run would post its status with a more specific context key (e.g., "ApplicationName/PipelineRunName").
Because the GitLab API does not allow deleting commit pipeline jobs, the original, generic "failed" status remained on the commit forever. This resulted in the overall commit pipeline being permanently and incorrectly marked as "failed," even though all pipelines eventually succeeded.
The Solution:
this commit provides PipelineRun name from "OriginalPRName" annotation so that contextKey is uniform for "success" PipelineRun and failed PipelineRuns due to validation.
Before
After
After Fixing the PipelineRun syntax error here is how the status is update with correct job name
📝 Description of the Change
👨🏻 Linked Jira
https://issues.redhat.com/browse/SRVKP-9044
🔗 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.