optimize the deletion order in the unpublish <extension> command.#123
Conversation
kubesphere-prow
Bot
added
release-note
size/M
![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 updates the unpublish command to handle the deletion of InstallPlan objects synchronously by waiting for their removal. It also refactors the code to pass a context through deletion functions. Feedback includes using the command's context for better interrupt handling, utilizing idiomatic helper functions for object keys, and optimizing the polling loop by moving object cloning outside of it.
| } | ||
|
|
||
| func (o *unpublishOptions) unpublish(cmd *cobra.Command, args []string) error { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
It is recommended to use cmd.Context() instead of context.Background(). This allows the command to respect context cancellation, such as when a user interrupts the process (e.g., via Ctrl+C), ensuring a more responsive CLI experience.
| ctx := context.Background() | |
| ctx := cmd.Context() |
| return wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(ctx context.Context) (bool, error) { | ||
| current, ok := obj.DeepCopyObject().(runtimeclient.Object) | ||
| if !ok { | ||
| return false, fmt.Errorf("object %T does not implement client.Object", obj) | ||
| } | ||
| if err := c.Get(ctx, key, current); err != nil { | ||
| if errors.IsNotFound(err) { | ||
| return true, nil | ||
| } | ||
| return false, err | ||
| } | ||
| return false, nil | ||
| }) |
There was a problem hiding this comment.
The object cloning and type assertion are performed inside the polling loop. Moving these operations outside the loop improves efficiency by avoiding redundant allocations and checks in every iteration.
| return wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(ctx context.Context) (bool, error) { | |
| current, ok := obj.DeepCopyObject().(runtimeclient.Object) | |
| if !ok { | |
| return false, fmt.Errorf("object %T does not implement client.Object", obj) | |
| } | |
| if err := c.Get(ctx, key, current); err != nil { | |
| if errors.IsNotFound(err) { | |
| return true, nil | |
| } | |
| return false, err | |
| } | |
| return false, nil | |
| }) | |
| current, ok := obj.DeepCopyObject().(runtimeclient.Object) | |
| if !ok { | |
| return fmt.Errorf("object %T does not implement client.Object", obj) | |
| } | |
| return wait.PollUntilContextTimeout(ctx, interval, timeout, true, func(ctx context.Context) (bool, error) { | |
| if err := c.Get(ctx, key, current); err != nil { | |
| if errors.IsNotFound(err) { | |
| return true, nil | |
| } | |
| return false, err | |
| } | |
| return false, nil | |
| }) |
Signed-off-by: frezes <zhangjunhao@kubesphere.io>
|
/lgtm |
|
LGTM label has been added. DetailsGit tree hash: 26eef6816c7b70bbc9ed54d896ee25e79bc1a7fb |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: frezes, redscholar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubesphere-prow
Bot
added
the
approved
3 participants
What this PR does / why we need it:
In versions below KSE v4.2.1, the controller queries the corresponding ExtensionVersion before deleting the InstallPlan. If the ExtensionVersion has already been deleted, the InstallPlan deletion will be blocked and cannot be cleaned up properly. Therefore, the deletion order needs to be optimized.
Does this PR introduced a user-facing change?