feat: SKR Webhook & Watcher Cert Setup Deletion #2906
Conversation
lindnerby
changed the title
lindnerby
changed the title
| name: "Nothing for UseCaseDeleteWatcherCertificate", | ||
| useCase: usecase.DeleteWatcherCertificate, |
There was a problem hiding this comment.
| name: "Nothing for UseCaseDeleteWatcherCertificateSetup", | |
| useCase: usecase.DeleteWatcherCertificateSetup, |
| if r.WatcherEnabled() { | ||
| if err := r.SKRWebhookManager.Remove(ctx, kyma); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this one we wanted to keep for now?
| } | ||
|
|
||
| func (u *RemoveSkrWebhookResources) IsApplicable(ctx context.Context, kyma *v1beta2.Kyma) (bool, error) { | ||
| if kyma.DeletionTimestamp.IsZero() || kyma.Status.State != shared.StateDeleting { |
There was a problem hiding this comment.
In the other use cases, I only checked for the deletion timestamp, not the state. Do you think we should introduce this state check for all other use cases as well?
|
|
||
| resourcesExist, err := u.skrWebhookResourcesRepo.ResourcesExist(kyma.Name) | ||
| if err != nil { | ||
| return false, errors.Join(err, errFailedToDetermineApplicability) |
There was a problem hiding this comment.
Should this rather be errors.Join(errFailedToDetermineApplicability, err)? If I am not wrong, calling Error() on the resulting error would read "err.Err() => errFailedToDetermineApplicability.Err()" so oldest to newest instead of newest to oldest.
| if err != nil { | ||
| return result.Result{ | ||
| UseCase: u.Name(), | ||
| Err: errors.Join(err, errFailedToRemoveSkrWebhookResources), |
| resourceExists := make(chan bool, 1) | ||
|
|
||
| for idx := range r.resources { | ||
| resIdx := idx |
| Namespace: r.remoteSyncNamespace, | ||
| } | ||
|
|
||
| skrClient := r.skrClientCache.Get(kymaNamespacedName) |
| errGrp, grpCtx := errgroup.WithContext(ctx) | ||
|
|
||
| for idx := range r.resources { | ||
| resIdx := idx |
| err := skrClient.Delete(grpCtx, resource) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to delete resource %s: %w", ref.Name, err) | ||
| } |
There was a problem hiding this comment.
I think we already need to ignore not found here. If I understant the errGrp.Wait correclty, it returns the first non nill error. If we have something like:
- err1: not found
- err2: some real error
Then the err below is err1 and since we ignore not found there, we would miss the real error in err2.
| assert.Contains(t, err.Error(), "failed to check resource") | ||
| }) | ||
|
|
||
| t.Run("short-circuits on first found resource", func(t *testing.T) { |
There was a problem hiding this comment.
What is this test adding on to pof the returns true when at least one resource exists?
2 participants
Description
Changes proposed in this pull request: