From 180a08c8515f7bdf4b32f5a05364816392e9eba9 Mon Sep 17 00:00:00 2001 From: Sean Marciniak <30928402+MovieStoreGuy@users.noreply.github.com> Date: Fri, 25 Oct 2024 09:29:03 +1030 Subject: [PATCH] Enhancement: standardise error handling (#539) * Providing a standardise way to handle errors * Fixing up error handling * Renaming to handle error --- internal/common/error.go | 48 +++++++++++++++++++++ internal/common/error_test.go | 54 ++++++++++++++++++++++++ internal/definition/detector/resource.go | 10 +++-- internal/definition/team/resource.go | 12 ++++-- 4 files changed, 116 insertions(+), 8 deletions(-) create mode 100644 internal/common/error.go create mode 100644 internal/common/error_test.go diff --git a/internal/common/error.go b/internal/common/error.go new file mode 100644 index 00000000..92b90edb --- /dev/null +++ b/internal/common/error.go @@ -0,0 +1,48 @@ +// Copyright Splunk, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package common + +import ( + "context" + "net/http" + + "github.com/hashicorp/terraform-plugin-log/tflog" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/signalfx/signalfx-go" + + tfext "github.com/splunk-terraform/terraform-provider-signalfx/internal/tfextension" +) + +// HandleError handles the general case when the signalfx api returns +// an error, and it uses that information to determine what needs to happen. +// This will ensure that the state is cleaned up given the error condition. +// To help simplify error handling, it will always return the error provided. +func HandleError(ctx context.Context, err error, data *schema.ResourceData) error { + re, ok := signalfx.AsResponseError(err) + if !ok { + // Not a response error, pass it back + return err + } + switch re.Code() { + case http.StatusNotFound: + tflog.Info(ctx, "Resource has been removed externally, removing from state", tfext.NewLogFields(). + Field("route", re.Route()), + ) + // Clear the id from the state when 404 is returned. + data.SetId("") + case http.StatusUnauthorized: + tflog.Error(ctx, "Token is not authorized", tfext.NewLogFields(). + Field("route", re.Route()). + Field("code", re.Code()). + Field("details", re.Details()), + ) + default: + tflog.Debug(ctx, "Issue trying to work with the API", tfext.NewLogFields(). + Field("route", re.Route()). + Field("code", re.Code()). + Field("details", re.Details()), + ) + } + return err +} diff --git a/internal/common/error_test.go b/internal/common/error_test.go new file mode 100644 index 00000000..a15e6991 --- /dev/null +++ b/internal/common/error_test.go @@ -0,0 +1,54 @@ +// Copyright Splunk, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package common + +import ( + "context" + "errors" + "testing" + + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/signalfx/signalfx-go" + "github.com/stretchr/testify/assert" +) + +func TestOnError(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + err error + expect string + }{ + { + name: "no error provided", + err: nil, + expect: "id", + }, + { + name: "not a response rror", + err: errors.New("derp"), + expect: "id", + }, + { + name: "uncatalogue response error", + err: &signalfx.ResponseError{}, + expect: "id", + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + data := schema.TestResourceDataRaw( + t, + map[string]*schema.Schema{}, + map[string]any{}, + ) + data.SetId("id") + + assert.ErrorIs(t, tc.err, HandleError(context.Background(), tc.err, data), "Must return the same error") + assert.Equal(t, tc.expect, data.Id(), "Must have the expected id") + }) + } +} diff --git a/internal/definition/detector/resource.go b/internal/definition/detector/resource.go index b9cdbf2d..c7b76911 100644 --- a/internal/definition/detector/resource.go +++ b/internal/definition/detector/resource.go @@ -12,6 +12,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/signalfx/signalfx-go/detector" + "github.com/splunk-terraform/terraform-provider-signalfx/internal/common" pmeta "github.com/splunk-terraform/terraform-provider-signalfx/internal/providermeta" tfext "github.com/splunk-terraform/terraform-provider-signalfx/internal/tfextension" ) @@ -63,7 +64,7 @@ func resourceCreate(ctx context.Context, data *schema.ResourceData, meta any) (i ParentDetectorId: dt.ParentDetectorId, DetectorOrigin: dt.DetectorOrigin, }) - if err != nil { + if common.HandleError(ctx, err, data) != nil { return tfext.AsErrorDiagnostics(err) } @@ -88,7 +89,7 @@ func resourceRead(ctx context.Context, data *schema.ResourceData, meta any) (iss } dt, err := client.GetDetector(ctx, data.Id()) - if err != nil { + if common.HandleError(ctx, err, data) != nil { return tfext.AsErrorDiagnostics(err) } @@ -133,7 +134,7 @@ func resourceUpdate(ctx context.Context, data *schema.ResourceData, meta any) (i ParentDetectorId: dt.ParentDetectorId, DetectorOrigin: dt.DetectorOrigin, }) - if err != nil { + if common.HandleError(ctx, err, data) != nil { return tfext.AsErrorDiagnostics(err) } @@ -154,5 +155,6 @@ func resourceDelete(ctx context.Context, data *schema.ResourceData, meta any) di if err != nil { return tfext.AsErrorDiagnostics(err) } - return tfext.AsErrorDiagnostics(client.DeleteDetector(ctx, data.Id())) + err = common.HandleError(ctx, client.DeleteDetector(ctx, data.Id()), data) + return tfext.AsErrorDiagnostics(err) } diff --git a/internal/definition/team/resource.go b/internal/definition/team/resource.go index f91262bf..1cd916d9 100644 --- a/internal/definition/team/resource.go +++ b/internal/definition/team/resource.go @@ -11,6 +11,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/signalfx/signalfx-go/team" + "github.com/splunk-terraform/terraform-provider-signalfx/internal/common" pmeta "github.com/splunk-terraform/terraform-provider-signalfx/internal/providermeta" ) @@ -49,7 +50,7 @@ func newResourceCreate() schema.CreateContextFunc { Members: payload.Members, NotificationLists: payload.NotificationLists, }) - if err != nil { + if common.HandleError(ctx, err, rd) != nil { return diag.FromErr(err) } @@ -73,9 +74,10 @@ func newResourceRead() schema.ReadContextFunc { } tm, err := client.GetTeam(ctx, rd.Id()) - if err != nil { + if common.HandleError(ctx, err, rd) != nil { return diag.FromErr(err) } + tflog.Debug(ctx, "Successfully fetched team data") if err := rd.Set("url", pmeta.LoadApplicationURL(ctx, meta, AppPath, tm.Id)); err != nil { @@ -104,7 +106,7 @@ func newResourceUpdate() schema.UpdateContextFunc { Members: payload.Members, NotificationLists: payload.NotificationLists, }) - if err != nil { + if common.HandleError(ctx, err, rd) != nil { return diag.FromErr(err) } @@ -128,6 +130,8 @@ func newResourceDelete() schema.DeleteContextFunc { "team-id": rd.Id(), }) - return diag.FromErr(client.DeleteTeam(ctx, rd.Id())) + err = common.HandleError(ctx, client.DeleteTeam(ctx, rd.Id()), rd) + + return diag.FromErr(err) } }