From 110f20aa81f79ad1bc21f3280aa08d36924b876e Mon Sep 17 00:00:00 2001 From: Tomoya Nishide Date: Sun, 29 Mar 2020 12:07:42 +0900 Subject: [PATCH 1/2] return event with EventTypeNormal in Finalize func --- github/pkg/reconciler/githubsource.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/github/pkg/reconciler/githubsource.go b/github/pkg/reconciler/githubsource.go index 8b725e8f74..88ef25e86b 100644 --- a/github/pkg/reconciler/githubsource.go +++ b/github/pkg/reconciler/githubsource.go @@ -173,8 +173,9 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, source *sourcesv1alpha1.G if err != nil { source.Status.MarkNoSecrets("AccessTokenNotFound", "%s", err) 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) } args := &webhookArgs{ @@ -186,8 +187,9 @@ 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 + 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) } // Webhook deleted, clear ID source.Status.WebhookIDKey = "" From f1e2bc6620c857bb673d32e118ec4f7341c7235b Mon Sep 17 00:00:00 2001 From: Tomoya Nishide Date: Tue, 31 Mar 2020 22:42:36 +0900 Subject: [PATCH 2/2] error chain handling --- github/pkg/reconciler/githubsource.go | 27 +++++++++++++++++++------ github/pkg/reconciler/webhook_client.go | 2 +- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/github/pkg/reconciler/githubsource.go b/github/pkg/reconciler/githubsource.go index 88ef25e86b..76b1d77025 100644 --- a/github/pkg/reconciler/githubsource.go +++ b/github/pkg/reconciler/githubsource.go @@ -18,7 +18,9 @@ package github import ( "context" + "errors" "fmt" + "net/http" "strings" //k8s.io imports @@ -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 ( @@ -170,12 +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) 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 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{ @@ -186,10 +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, "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) + 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 = "" @@ -292,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 } diff --git a/github/pkg/reconciler/webhook_client.go b/github/pkg/reconciler/webhook_client.go index e94d10e310..f6966fc202 100644 --- a/github/pkg/reconciler/webhook_client.go +++ b/github/pkg/reconciler/webhook_client.go @@ -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)