Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

Remove finalizer to avoid finalize loop in GitHubSource FinalizeKind(). #1089

Merged
merged 2 commits into from
Apr 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 24 additions & 7 deletions github/pkg/reconciler/githubsource.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ package github

import (
"context"
"errors"
"fmt"
"net/http"
"strings"

//k8s.io imports
Expand Down Expand Up @@ -46,6 +48,8 @@ import (
"knative.dev/pkg/logging"
pkgreconciler "knative.dev/pkg/reconciler"
"knative.dev/pkg/resolver"

ghclient "github.com/google/go-github/github"
)

const (
Expand Down Expand Up @@ -170,11 +174,16 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, source *sourcesv1alpha1.G
if source.Status.WebhookIDKey != "" {
// Get access token
accessToken, err := r.secretFrom(ctx, source.Namespace, source.Spec.AccessToken.SecretKeyRef)
if err != nil {
if apierrors.IsNotFound(err) {
source.Status.MarkNoSecrets("AccessTokenNotFound", "%s", err)
tom24d marked this conversation as resolved.
Show resolved Hide resolved
controller.GetEventRecorder(ctx).Eventf(source, corev1.EventTypeWarning,
"FailedFinalize", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
return err
"WebhookDeletionSkipped", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
// return EventTypeNormal to avoid finalize loop
return pkgreconciler.NewEvent(corev1.EventTypeNormal, "WebhookDeletionSkipped", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
} else if err != nil {
controller.GetEventRecorder(ctx).Eventf(source, corev1.EventTypeWarning,
"WebhookDeletionFailed", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
return fmt.Errorf("error getting secret: %v", err)
}

args := &webhookArgs{
Expand All @@ -185,9 +194,17 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, source *sourcesv1alpha1.G
}
// Delete the webhook using the access token and stored webhook ID
err = r.deleteWebhook(ctx, args)
if err != nil {
controller.GetEventRecorder(ctx).Eventf(source, corev1.EventTypeWarning, "FailedFinalize", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
return err
var gherr *ghclient.ErrorResponse
if errors.As(err, &gherr) {
if gherr.Response.StatusCode == http.StatusNotFound {
controller.GetEventRecorder(ctx).Eventf(source, corev1.EventTypeWarning, "WebhookDeletionSkipped", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
// return EventTypeNormal to avoid finalize loop
return pkgreconciler.NewEvent(corev1.EventTypeNormal, "WebhookDeletionSkipped", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
}
} else {
controller.GetEventRecorder(ctx).Eventf(source, corev1.EventTypeWarning,
"WebhookDeletionFailed", "Could not delete webhook %q: %v", source.Status.WebhookIDKey, err)
return fmt.Errorf("error deleting webhook: %v", err)
}
// Webhook deleted, clear ID
source.Status.WebhookIDKey = ""
Expand Down Expand Up @@ -290,7 +307,7 @@ func (r *Reconciler) deleteWebhook(ctx context.Context, args *webhookArgs) error
}
err = r.webhookClient.Delete(ctx, hookOptions, args.hookID, args.alternateGitHubAPIURL)
if err != nil {
return fmt.Errorf("failed to delete webhook: %v", err)
return fmt.Errorf("failed to delete webhook: %w", err)
}
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion github/pkg/reconciler/webhook_client.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ func (client gitHubWebhookClient) Delete(ctx context.Context, options *webhookOp
}
if err != nil {
logger.Infof("delete webhook error response:\n%+v", resp)
return fmt.Errorf("failed to delete the webhook: %v", err)
return fmt.Errorf("failed to delete the webhook: %w", err)
}

logger.Infof("deleted hook: %s", hook.ID)
Expand Down