fix: report cancelled status when PipelineRun is deleted #2349
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 addresses a bug where the status of a PipelineRun on Git providers (like GitHub) would remain stuck in 'Pending' or 'Running' if the PipelineRun was deleted before completion. The changes introduce a mechanism to ensure that when a PipelineRun is deleted, its status is accurately updated to 'cancelled' on the Git provider, providing a more precise reflection of the PipelineRun's lifecycle and improving user experience. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue where deleting a 'Running' or 'Queued' PipelineRun would leave its status stuck on the Git provider. The fix introduces logic in the finalizer to report a 'cancelled' status in this scenario, which is a good approach. The related refactoring to extract initGitProviderClient is also a positive change that improves code reuse.
My review includes a critical fix for an error handling issue where a failure to initialize the Git provider client is silently ignored, which could prevent the 'cancelled' status from being reported. I've also included suggestions to improve logging style and to strengthen the new unit test to make it more robust and less brittle.
|
/test |
🔍 PR Lint Feedback
|
zakisk
force-pushed
the
SRVKP-8318-prevent-zombie-pipeline-status
branch
from
4969c51 to
c3861d4
Compare
zakisk
force-pushed
the
SRVKP-8318-prevent-zombie-pipeline-status
branch
2 times, most recently
from
573f95a to
8cfdc21
Compare
When a PipelineRun is deleted while it is in 'Running' or 'Queued' state (i.e., not yet completed), the status on the Git provider (e.g., GitHub) remained stuck in 'Pending' or 'Running'. This commit adds logic to the Finalizer to detect when a PipelineRun is being deleted (finalized) while in an active state. It now: 1. Reports a "cancelled" status to the Git provider, ensuring the commit status is correctly updated to "cancelled" instead of hanging. 2. Added unit test case to confirm that status is reported cancelled. This improves the user experience by accurately reflecting the PipelineRun lifecycle events on the Git provider UI. https://issues.redhat.com/browse/SRVKP-8318 Signed-off-by: Zaki Shaikh <zashaikh@redhat.com>
zakisk
force-pushed
the
SRVKP-8318-prevent-zombie-pipeline-status
branch
from
8cfdc21 to
73bd006
Compare
aThorp96
left a comment
aThorp96
left a comment
There was a problem hiding this comment.
This will help the reliability a lot. Thanks! A couple suggestions and thoughts
| return nil | ||
| } | ||
|
|
||
| func (r *Reconciler) reportPipelineRunAsCancelled(ctx context.Context, logger *zap.SugaredLogger, repo *v1alpha1.Repository, pr *tektonv1.PipelineRun) error { |
There was a problem hiding this comment.
In other functions we use logger := logging.FromContext(ctx) instead of passing in a logger. Any reason we're not doing that here?
| // report the PipelineRun as cancelled as it was queued or started but it is deleted | ||
| // but its status is still in progress or queued on git provider | ||
| if err := r.reportPipelineRunAsCancelled(ctx, logger, repo, pr); err != nil { | ||
| logger.Errorf("failed to report started pipeline run as cancelled: %w", err) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Can this be defered as soon as we confirm the Repository? SInce there are some fast-returns inside this if-statement there's several happy paths and failure paths which could cause this to get skipped
| // but its status is still in progress or queued on git provider | ||
| if err := r.reportPipelineRunAsCancelled(ctx, logger, repo, pr); err != nil { | ||
| logger.Errorf("failed to report started pipeline run as cancelled: %w", err) | ||
| return err |
There was a problem hiding this comment.
I'm not sure if return err is the right move here, since it will block the finalizer from being cleared. On one hand, that will help ensure the status is updated. On the other hand though, if for example a repository CR's API key expires or is rate limited, its PLRs effectively can't be deleted. WDYT?
There was a problem hiding this comment.
yeah, I was also skeptic about it that whether we should error out on api failure or not 🤔 but now I'm more inclined towards to not returning error. I will update this
| } | ||
| detectedProvider.SetPacInfo(&pacInfo) | ||
|
|
||
| if event.InstallationID > 0 { |
There was a problem hiding this comment.
Non-blocking suggestion
| if event.InstallationID > 0 { | |
| // installation ID indicated Github App installation | |
| if event.InstallationID > 0 { |
| if event.InstallationID > 0 { | ||
| event.Provider.WebhookSecret, _ = pac.GetCurrentNSWebhookSecret(ctx, r.kinteract, r.run) | ||
| } else { | ||
| // secretNS is needed when git provider is other than Github. |
There was a problem hiding this comment.
| // secretNS is needed when git provider is other than Github. | |
| // secretNS is needed when git provider is other than Github App. |
3 participants
When a PipelineRun is deleted while it is in 'Running' or 'Queued' state (i.e., not yet completed), the status on the Git provider (e.g., GitHub) remained stuck in 'Pending' or 'Running'.
This commit adds logic to the Finalizer to detect when a PipelineRun is being deleted (finalized) while in an active state. It now:
This improves the user experience by accurately reflecting the PipelineRun lifecycle events on the Git provider UI.
https://issues.redhat.com/browse/SRVKP-8318
📝 Description of the Change
👨🏻 Linked Jira
🔗 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-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.