Fix: do not retry for 4xx errors

env0/provider.go

@@ -171,57 +171,55 @@ func Provider(version string) plugin.ProviderFunc {
 	}
 }
 
-func configureProvider(version string, p *schema.Provider) schema.ConfigureContextFunc {
-	userAgent := p.UserAgent("terraform-provider-env0", version)
-
-	return func(ctx context.Context, d *schema.ResourceData) (interface{}, diag.Diagnostics) {
-		restyClient := resty.New()
+func createRestyClient(ctx context.Context) *resty.Client {
+	isIntegrationTest := false
 
-		isIntegrationTest := false
+	if os.Getenv("INTEGRATION_TESTS") == "1" {
+		isIntegrationTest = true
+	}
 
-		if os.Getenv("INTEGRATION_TESTS") == "1" {
-			isIntegrationTest = true
-		}
+	subCtx := tflog.NewSubsystem(ctx, "env0_api_client")
+
+	return resty.New().SetRetryCount(5).
+		SetRetryWaitTime(time.Second).
+		SetRetryMaxWaitTime(time.Second * 5).
+		OnBeforeRequest(func(c *resty.Client, r *resty.Request) error {
+			if r != nil {
+				tflog.SubsystemInfo(subCtx, "env0_api_client", "Sending request", map[string]interface{}{"method": r.Method, "url": r.URL})
+			}
+			return nil
+		}).
+		OnAfterResponse(func(c *resty.Client, r *resty.Response) error {
+			tflog.SubsystemInfo(subCtx, "env0_api_client", "Received response", map[string]interface{}{"method": r.Request.Method, "url": r.Request.URL, "status": r.Status()})
+			return nil
+		}).
+		AddRetryCondition(func(r *resty.Response, err error) bool {
+			if r == nil {
+				// No response. Possibly a networking issue (E.g. DNS lookup failure).
+				tflog.SubsystemWarn(subCtx, "env0_api_client", "No response, retrying request")
+				return true
+			}
+
+			// When running integration tests 404 may occur due to "database eventual consistency".
+			// Retry when there's a 5xx error. Otherwise do not retry.
+			if r.StatusCode() >= 500 || (isIntegrationTest && r.StatusCode() == 404) {
+				tflog.SubsystemWarn(subCtx, "env0_api_client", "Received a failed or not found response, retrying request", map[string]interface{}{"method": r.Request.Method, "url": r.Request.URL, "status code": r.StatusCode()})
+				return true
+			}
+
+			if r.StatusCode() == 200 && isIntegrationTest && r.String() == "[]" {
+				tflog.SubsystemWarn(subCtx, "env0_api_client", "Received an empty list , retrying request", map[string]interface{}{"method": r.Request.Method, "url": r.Request.URL})
+				return true
+			}
+
+			return false
+		})
+}
 
-		subCtx := tflog.NewSubsystem(ctx, "env0_api_client")
-
-		restyClient = restyClient.
-			SetRetryCount(5).
-			SetRetryWaitTime(time.Second).
-			SetRetryMaxWaitTime(time.Second * 5).
-			OnBeforeRequest(func(c *resty.Client, r *resty.Request) error {
-				if r != nil {
-					tflog.SubsystemInfo(subCtx, "env0_api_client", "Sending request", map[string]interface{}{"method": r.Method, "url": r.URL})
-				}
-				return nil
-			}).
-			OnAfterResponse(func(c *resty.Client, r *resty.Response) error {
-				tflog.SubsystemInfo(subCtx, "env0_api_client", "Received response", map[string]interface{}{"method": r.Request.Method, "url": r.Request.URL, "status": r.Status()})
-				return nil
-			}).
-			AddRetryAfterErrorCondition().
-			AddRetryCondition(func(r *resty.Response, err error) bool {
-				if r == nil {
-					// No response. Possibly a networking issue (E.g. DNS lookup failure).
-					tflog.SubsystemWarn(subCtx, "env0_api_client", "No response, retrying request")
-					return true
-				}
-
-				// When running integration tests 404 may occur due to "database eventual consistency".
-				// Retry when there's a 5xx error. Otherwise do not retry.
-				if r.StatusCode() >= 500 || (isIntegrationTest && r.StatusCode() == 404) {
-					tflog.SubsystemWarn(subCtx, "env0_api_client", "Received a failed or not found response, retrying request", map[string]interface{}{"method": r.Request.Method, "url": r.Request.URL, "status code": r.StatusCode()})
-					return true
-				}
-
-				if r.StatusCode() == 200 && isIntegrationTest && r.String() == "[]" {
-					tflog.SubsystemWarn(subCtx, "env0_api_client", "Received an empty list , retrying request", map[string]interface{}{"method": r.Request.Method, "url": r.Request.URL})
-					return true
-				}
-
-				return false
-			})
+func configureProvider(version string, p *schema.Provider) schema.ConfigureContextFunc {
+	userAgent := p.UserAgent("terraform-provider-env0", version)
 
+	return func(ctx context.Context, d *schema.ResourceData) (interface{}, diag.Diagnostics) {
 		apiKey, ok := d.GetOk("api_key")
 		if !ok {
 			return nil, diag.Diagnostics{diag.Diagnostic{Severity: diag.Error, Detail: `The argument "api_key" is required, but no definition was found.`}}
@@ -237,7 +235,7 @@ func configureProvider(version string, p *schema.Provider) schema.ConfigureConte
 			ApiSecret:   apiSecret.(string),
 			ApiEndpoint: d.Get("api_endpoint").(string),
 			UserAgent:   userAgent,
-			RestClient:  restyClient,
+			RestClient:  createRestyClient(ctx),
 		})
 		if err != nil {
 			return nil, diag.Diagnostics{diag.Diagnostic{Severity: diag.Error, Summary: err.Error()}}
@@ -249,6 +247,7 @@ func configureProvider(version string, p *schema.Provider) schema.ConfigureConte
 		if _, err := apiClient.OrganizationId(); err != nil {
 			return nil, diag.Diagnostics{diag.Diagnostic{Severity: diag.Error, Summary: err.Error()}}
 		}
+
 		return apiClient, nil
 	}
 }

env0/provider_test.go

@@ -3,19 +3,30 @@ package env0
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"os"
 	"strings"
 	"testing"
 
 	"github.com/env0/terraform-provider-env0/client"
 	"github.com/env0/terraform-provider-env0/utils"
+	"github.com/go-resty/resty/v2"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+	"github.com/jarcoal/httpmock"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/suite"
 	"go.uber.org/mock/gomock"
 )
 
+type testRestyClientSuite struct {
+	suite.Suite
+	client *resty.Client
+	url    string
+}
+
 func runUnitTest(t *testing.T, testCase resource.TestCase, mockFunc func(mockFunc *client.MockApiClientInterface)) {
 	t.Helper()
 
@@ -113,3 +124,70 @@ func TestMissingConfigurations(t *testing.T) {
 		testMissingEnvVar(t, envVars, expectedError)
 	}
 }
+
+func (suite *testRestyClientSuite) SetupTest() {
+	httpmock.Reset()
+}
+
+func (suite *testRestyClientSuite) SetupSuite() {
+	httpmock.ActivateNonDefault(suite.client.GetClient())
+}
+
+func (suite *testRestyClientSuite) TearDownAllSuite() {
+	httpmock.Deactivate()
+}
+
+func (suite *testRestyClientSuite) TestOkResponse() {
+	t := suite.T()
+
+	httpmock.RegisterResponder("GET", suite.url, httpmock.NewStringResponder(http.StatusOK, "OK"))
+
+	res, err := suite.client.R().Get(suite.url)
+
+	if assert.NoError(t, err) {
+		assert.Equal(t, http.StatusOK, res.StatusCode())
+		assert.Equal(t, "OK", res.String())
+	}
+
+	assert.Equal(t, 1, httpmock.GetTotalCallCount())
+}
+
+func (suite *testRestyClientSuite) Test4xxResponse() {
+	t := suite.T()
+
+	httpmock.RegisterResponder("GET", suite.url, httpmock.NewStringResponder(http.StatusBadRequest, "BAD"))
+
+	res, err := suite.client.R().Get(suite.url)
+
+	if assert.NoError(t, err) {
+		assert.Equal(t, http.StatusBadRequest, res.StatusCode())
+		assert.Equal(t, "BAD", res.String())
+	}
+
+	// Should be called once - no retries.
+	assert.Equal(t, 1, httpmock.GetTotalCallCount())
+}
+
+func (suite *testRestyClientSuite) Test5xxResponse() {
+	t := suite.T()
+
+	httpmock.RegisterResponder("GET", suite.url, httpmock.NewStringResponder(http.StatusInternalServerError, "BAD"))
+
+	res, err := suite.client.R().Get(suite.url)
+
+	if assert.NoError(t, err) {
+		assert.Equal(t, http.StatusInternalServerError, res.StatusCode())
+		assert.Equal(t, "BAD", res.String())
+	}
+
+	// Should be called multiple times - retries.
+	assert.Equal(t, 6, httpmock.GetTotalCallCount())
+}
+
+func TestRestyClientSuite(t *testing.T) {
+	s := &testRestyClientSuite{
+		client: createRestyClient(context.Background()),
+		url:    "http://fake.env0.com/fake",
+	}
+	suite.Run(t, s)
+}