Conversation
Summary of ChangesHello, 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 introduces the capability to delete PipelineRun resources, providing a new API endpoint for this operation. It includes robust logic to prevent accidental deletion of active pipeline runs and offers flexibility to retain associated Jenkins build records if desired. The changes ensure proper resource lifecycle management and cleanup within the system. Highlights
Changelog
Activity
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.
Code Review
This pull request adds support for deleting PipelineRun resources. The implementation includes a new API endpoint for deletion, with an option to keep the corresponding Jenkins job history. The controller logic is updated with a finalizer to handle the cleanup of Jenkins resources upon PipelineRun deletion. The changes are well-structured and the logic is sound. I have a couple of suggestions to improve the robustness of the controller's update operations.
| pipelineRunCopied.ObjectMeta.Finalizers = sliceutil.RemoveString(pipelineRunCopied.ObjectMeta.Finalizers, func(item string) bool { | ||
| return item == v1alpha3.PipelineRunFinalizerName | ||
| }) | ||
| err = r.Update(context.TODO(), pipelineRunCopied) |
There was a problem hiding this comment.
This update to remove the finalizer should be wrapped in a retry.RetryOnConflict loop to handle potential conflicts gracefully. This is a standard pattern in controllers to avoid reconciliation failures due to object modifications between read and write operations. You can see an example of this pattern in the updateStatus function in this file.
Additionally, please use the ctx from the Reconcile function instead of context.TODO().
| // add finalizer if it does not exist | ||
| if !sliceutil.HasString(pipelineRunCopied.ObjectMeta.Finalizers, v1alpha3.PipelineRunFinalizerName) { | ||
| pipelineRunCopied.ObjectMeta.Finalizers = append(pipelineRunCopied.ObjectMeta.Finalizers, v1alpha3.PipelineRunFinalizerName) | ||
| if err := r.Update(ctx, pipelineRunCopied); err != nil { |
There was a problem hiding this comment.
This update to add the finalizer should be wrapped in a retry.RetryOnConflict loop to handle potential conflicts gracefully. This is a standard pattern in controllers to avoid reconciliation failures due to object modifications between read and write operations. You can see an example of this pattern in the updateStatus function in this file.
Signed-off-by: renyunkang <rykren1998@gmail.com>
What type of PR is this?
/kind feature
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes # https://github.com/kubesphere/project/issues/7185
Special notes for reviewers:
Please check the following list before waiting reviewers:
Does this PR introduce a user-facing change??