golangci - enable tenv linter and swap os.SetEnv to correct t.SetEnv …

…& enabled (hashicorp#28005)

* golangci - enable tenv and swap os.SetEnv to correct t.SetEnv

* enabled nilerr

* disable nilerr

* re-enable nilerr

* fix test

* make test

* switch back to os for as sdkv2 is parallel

* Update internal/sdk/plugin_sdk_test.go

* fix nolint

.golangci.yml

@@ -32,37 +32,32 @@ linters:
     - govet # reports suspicious constructs. It is roughly the same as 'go vet' (replaced vet and vetshadow)
     - ineffassign # detects when assignments to existing variables are not used
     - misspell # finds commonly misspelled English words.
-    #- nilerr # Finds the code that returns nil even if it checks that the error is not nil.
-    #- nlreturn # Nlreturn checks for a new line before return and branch statements to increase code clarity.
+    - nilerr # Finds code that returns nil after it checks that the error is not nil.
     - prealloc # finds slice declarations that could potentially be pre-allocated.
     - predeclared # find code that shadows one of Go's predeclared identifiers.
     - reassign # checks that package variables are not reassigned.
-    #- revive #Fast, configurable, extensible, flexible, and beautiful linter for Go. Drop-in replacement of golint.
+    - tenv #detects using os.Setenv instead of t.Setenv since Go1.17.
     - tagalign #  checks struct tags that do not align with the specified column in struct definitions.
     - staticcheck # checks rules from staticcheck. It's not the same thing as the staticcheck binary.
     - unused # checks Go code for unused constants, variables, functions and types.
     - unconvert # checks for unnecessary type conversions.
     - unparam # reports unused function parameters.
     - wastedassign # finds wasted assignment statements
     - whitespace # checks for unnecessary newlines at the start and end of functions, if, for, etc. (
-    #- wsl # add or remove empty lines.
 
-    ##### need to confirm these are valid and not false positives #####
+    ##### need to confirm these are valid and adding t.Parallel() to unit tests would be beneficial / integration tests would not be affected
     #- paralleltest # detects missing usage of t.Parallel() method in your Go test.
     #- tparallel # Tparallel detects inappropriate usage of t.Parallel() method in your Go test codes.
 
-    ###### DISABLED because : integer overflow conversion int -> int32
+    ###### DISABLED because : the number of possible integer overflow conversions from int -> int32. it's not an incorrect callout?
     # - gosec # Gosec is a security linter for Go source code
 
-    ##### DISABLED as i was fixing them with other linters, and not realizing how many there are - disabling for now even thou it is valid
+    ##### DISABLED as it (correctly) flags fmt.Errorf("constant") to be replaced with errors.New("constant") and there are ~1500 instances of this in the codebase
     #- perfsprint # Checks that fmt.Sprintf can be replaced with a faster alternative.
 
-    #### VALID but DISABLED as we have a lot of these in the codebase to switch over
+    #### DISABLED but valid as relying on output variable names is less than idea, have a lot of these in the codebase to switch over
     #- nakedret # Checks that functions with naked returns are not longer than a maximum size
 
-    #### DISABLED TO DO IN ITS OWN PR - should be enabled and done #####
-    #- tenv #detects using os.Setenv instead of t.Setenv since Go1.17.
-
     #### bunch of hits, need to confirm if errors or not #####
     #- copyloopvar #Detects range loop variables that are overwritten in the loop body
 
@@ -92,9 +87,4 @@ linters-settings:
       - tfschema
       - computed
   predeclared:
-    ignore: new,min,max # should we use newer, minimum, and maximum instead?
-  revive:
-    rules:
-      - name: var-naming
-        disabled: true
-
+    ignore: new,min,max # should we use newer, minimum, and maximum instead?

internal/provider/framework/config_test.go

@@ -27,7 +27,7 @@ func TestProviderConfig_LoadDefault(t *testing.T) {
 	}
 
 	// Skip enhanced validation
-	_ = os.Setenv("ARM_PROVIDER_ENHANCED_VALIDATION", "false")
+	t.Setenv("ARM_PROVIDER_ENHANCED_VALIDATION", "false")
 
 	testData := &ProviderModel{
 		ResourceProviderRegistrations: types.StringValue("none"),

internal/provider/framework/helpers_test.go

@@ -4,7 +4,6 @@
 package framework
 
 import (
-	"os"
 	"strings"
 	"testing"
 
@@ -70,10 +69,7 @@ func Test_getOidcTokenExpectMismatch(t *testing.T) {
 
 func Test_getOidcTokenAKSWorkload(t *testing.T) {
 	expectedString := "testOidcFromFile"
-	err := os.Setenv("AZURE_FEDERATED_TOKEN_FILE", "./testdata/oidc_test_input.txt")
-	if err != nil {
-		t.Fatalf("could not set env var (`AZURE_FEDERATED_TOKEN_FILE`) for test: %+v", err)
-	}
+	t.Setenv("AZURE_FEDERATED_TOKEN_FILE", "./testdata/oidc_test_input.txt")
 
 	p := &ProviderModel{
 		UseAKSWorkloadIdentity: basetypes.NewBoolValue(true),
@@ -91,17 +87,14 @@ func Test_getOidcTokenAKSWorkload(t *testing.T) {
 
 func Test_getOidcTokenAKSWorkloadExpectMismatch(t *testing.T) {
 	configuredString := "testOidc"
-	err := os.Setenv("AZURE_FEDERATED_TOKEN_FILE", "./testdata/oidc_test_input.txt")
-	if err != nil {
-		t.Fatalf("could not set env var (`AZURE_FEDERATED_TOKEN_FILE`) for test: %+v", err)
-	}
+	t.Setenv("AZURE_FEDERATED_TOKEN_FILE", "./testdata/oidc_test_input.txt")
 
 	p := &ProviderModel{
 		OIDCToken:              basetypes.NewStringValue(configuredString),
 		UseAKSWorkloadIdentity: basetypes.NewBoolValue(true),
 	}
 
-	_, err = getOidcToken(p)
+	_, err := getOidcToken(p)
 	if err == nil {
 		t.Fatal("expected an error but did not get one")
 	}
@@ -147,7 +140,7 @@ func Test_getClientSecretExpectMismatch(t *testing.T) {
 }
 
 func Test_getClientSecretFromFile(t *testing.T) {
-	os.Setenv("ARM_CLIENT_SECRET", "")
+	t.Setenv("ARM_CLIENT_SECRET", "")
 	expectedString := "testClientSecretFromFile"
 	p := &ProviderModel{
 		ClientSecretFilePath: basetypes.NewStringValue("./testdata/client_secret_test_input.txt"),
@@ -166,7 +159,7 @@ func Test_getClientSecretFromFile(t *testing.T) {
 }
 
 func Test_getClientSecretFromFileMismatch(t *testing.T) {
-	os.Setenv("ARM_CLIENT_SECRET", "foo")
+	t.Setenv("ARM_CLIENT_SECRET", "foo")
 	p := &ProviderModel{
 		ClientSecretFilePath: basetypes.NewStringValue("./testdata/client_secret_test_input.txt"),
 	}
@@ -233,14 +226,12 @@ func Test_getClientIDFromFile(t *testing.T) {
 
 func Test_getClientIDAKSWorkload(t *testing.T) {
 	expectedString := "testClientIDAKSWorkload"
-	err := os.Setenv("ARM_CLIENT_ID", expectedString)
-	if err != nil {
-		t.Fatalf("failed to set environment variable ARM_CLIENT_ID: %v", err)
-	}
+	t.Setenv("ARM_CLIENT_ID", expectedString)
 
 	p := &ProviderModel{
 		UseAKSWorkloadIdentity: basetypes.NewBoolValue(true),
 	}
+
 	result, err := getClientId(p)
 	if err != nil {
 		t.Fatalf("getClientID returned unexpected error %v", err)
@@ -255,16 +246,14 @@ func Test_getClientIDAKSWorkload(t *testing.T) {
 
 func Test_getClientIDAKSWorkloadExpectMismatch(t *testing.T) {
 	configuredString := "testClientIDAKSWorkload"
-	err := os.Setenv("ARM_CLIENT_ID", "testClientID")
-	if err != nil {
-		t.Fatalf("failed to set environment variable ARM_CLIENT_ID: %v", err)
-	}
+	t.Setenv("ARM_CLIENT_ID", "testClientID")
+
 	p := &ProviderModel{
 		ClientId:               basetypes.NewStringValue(configuredString),
 		UseAKSWorkloadIdentity: basetypes.NewBoolValue(true),
 	}
 
-	_, err = getClientId(p)
+	_, err := getClientId(p)
 	if err == nil {
 		t.Fatalf("expected an error but did not get one")
 	}

internal/sdk/plugin_sdk_test.go

@@ -15,7 +15,7 @@ import (
 )
 
 func TestAccPluginSDKAndDecoder(t *testing.T) {
-	os.Setenv("TF_ACC", "1")
+	os.Setenv("TF_ACC", "1") // nolint:tenv // plugin testing harness prevents this
 
 	type NestedType struct {
 		Key string `tfschema:"key"`
@@ -64,7 +64,7 @@ func TestAccPluginSDKAndDecoder(t *testing.T) {
 	}
 
 	// lintignore:AT001
-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{
 		ProviderFactories: map[string]func() (*schema.Provider, error){
 			"validator": func() (*schema.Provider, error) { //nolint:unparam
 				return &schema.Provider{
@@ -216,7 +216,7 @@ func TestAccPluginSDKAndDecoder(t *testing.T) {
 }
 
 func TestAccPluginSDKAndDecoderOptionalComputed(t *testing.T) {
-	os.Setenv("TF_ACC", "1")
+	os.Setenv("TF_ACC", "1") // nolint:tenv // plugin testing harness prevents this
 
 	type MyType struct {
 		Hello   string `tfschema:"hello"`
@@ -264,7 +264,7 @@ func TestAccPluginSDKAndDecoderOptionalComputed(t *testing.T) {
 	}
 
 	// lintignore:AT001
-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{
 		ProviderFactories: map[string]func() (*schema.Provider, error){
 			"validator": func() (*schema.Provider, error) { //nolint:unparam
 				return &schema.Provider{
@@ -340,7 +340,7 @@ resource "validator_decoder_unspecified" "test" {}
 }
 
 func TestAccPluginSDKAndDecoderOptionalComputedOverride(t *testing.T) {
-	os.Setenv("TF_ACC", "1")
+	os.Setenv("TF_ACC", "1") // nolint:tenv // plugin testing harness prevents this
 
 	type MyType struct {
 		Hello   string `tfschema:"hello"`
@@ -350,7 +350,7 @@ func TestAccPluginSDKAndDecoderOptionalComputedOverride(t *testing.T) {
 	}
 
 	// lintignore:AT001
-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{
 		ProviderFactories: map[string]func() (*schema.Provider, error){
 			"validator": func() (*schema.Provider, error) { //nolint:unparam
 				return &schema.Provider{
@@ -444,7 +444,7 @@ resource "validator_decoder_override" "test" {
 }
 
 func TestAccPluginSDKAndDecoderSets(t *testing.T) {
-	os.Setenv("TF_ACC", "1")
+	os.Setenv("TF_ACC", "1") // nolint:tenv // plugin testing harness prevents this
 
 	type MyType struct {
 		SetOfStrings []string  `tfschema:"set_of_strings"`
@@ -456,7 +456,7 @@ func TestAccPluginSDKAndDecoderSets(t *testing.T) {
 	}
 
 	// lintignore:AT001
-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{
 		ProviderFactories: map[string]func() (*schema.Provider, error){
 			"validator": func() (*schema.Provider, error) { //nolint:unparam
 				return &schema.Provider{
@@ -624,7 +624,7 @@ func TestAccPluginSDKAndDecoderSets(t *testing.T) {
 }
 
 func TestAccPluginSDKAndEncoder(t *testing.T) {
-	os.Setenv("TF_ACC", "1")
+	os.Setenv("TF_ACC", "1") // nolint:tenv // plugin testing harness prevents this
 
 	type NestedType struct {
 		Key string `tfschema:"key"`
@@ -649,7 +649,7 @@ func TestAccPluginSDKAndEncoder(t *testing.T) {
 	}
 
 	// lintignore:AT001
-	resource.ParallelTest(t, resource.TestCase{
+	resource.Test(t, resource.TestCase{
 		ProviderFactories: map[string]func() (*schema.Provider, error){
 			"validator": func() (*schema.Provider, error) { //nolint:unparam
 				return &schema.Provider{
@@ -863,7 +863,7 @@ func TestAccPluginSDKAndEncoder(t *testing.T) {
 }
 
 func TestAccPluginSDKReturnsComputedFields(t *testing.T) {
-	os.Setenv("TF_ACC", "1")
+	os.Setenv("TF_ACC", "1") // nolint:tenv // plugin sdk always is
 
 	resourceName := "validator_computed.test"
 	// lintignore:AT001

internal/services/apimanagement/migration/policy.go

@@ -22,7 +22,7 @@ func (ApiManagementPolicyV0ToV1) UpgradeFunc() pluginsdk.StateUpgraderFunc {
 	return func(ctx context.Context, rawState map[string]interface{}, meta interface{}) (map[string]interface{}, error) {
 		apiMgmtId, err := policy.ParseServiceID(rawState["id"].(string))
 		if err != nil {
-			return rawState, nil
+			return rawState, nil // lint:ignore nilerr this is not an error as we just want to skip the upgrade
 		}
 		id := policy.NewServiceID(apiMgmtId.SubscriptionId, apiMgmtId.ResourceGroupName, apiMgmtId.ServiceName)
 		rawState["id"] = id.ID()

internal/services/compute/capacity_reservation_group_resource.go

@@ -172,7 +172,7 @@ func resourceCapacityReservationGroupDelete(d *pluginsdk.ResourceData, meta inte
 			Refresh: func() (interface{}, string, error) {
 				res, err := client.Delete(ctx, *id)
 				if err != nil {
-					return res, "Deleting", nil
+					return res, "Deleting", nil // lint:ignore nilerr Returning nil error is intentional as we will retry the delete operation
 				}
 				return res, "Deleted", nil
 			},

internal/services/compute/no_downtime_resize.go

@@ -73,7 +73,7 @@ func determineIfVirtualMachineSkuSupportsNoDowntimeResize(ctx context.Context, v
 	virtualMachineId, err := virtualmachines.ParseVirtualMachineIDInsensitively(*virtualMachineIdRaw)
 	if err != nil {
 		log.Printf("[DEBUG] unable to parse Virtual Machine ID %q that the Managed Disk is attached too - skipping no-downtime-resize since we can't guarantee that's available", *virtualMachineIdRaw)
-		return pointer.To(false), nil
+		return pointer.To(false), nil // lint:ignore nilerr this is not an error as we just want to skip the check in this situation since we can't guarantee it's available
 	}
 
 	log.Printf("[DEBUG] Retrieving %s..", *virtualMachineId)

internal/services/storage/storage_account_static_website_data_plane_resource.go

@@ -195,8 +195,7 @@ func (a AccountStaticWebsiteResource) Delete() sdk.ResourceFunc {
 
 			accountDetails, err := storageClient.FindAccount(ctx, id.SubscriptionId, id.StorageAccountName)
 			if err != nil {
-				// If we don't find the account we can safely assume we don't need to remove the website since it must already be deleted
-				return nil
+				return nil // lint:ignore nilerr If we don't find the account we can safely assume we don't need to remove the website since it must already be deleted
 			}
 
 			properties := accounts.StorageServiceProperties{