chore: enable errcheck, nolintlint, and nilnil linters (#3122)

* Enable three new golangci-lint checks to improve error handling and code quality.

Ensures all error returns are explicitly handled or intentionally ignored to prevent silent failures.

**Fixed:**

- `internal/sanitizer/sanitizer.go` - Wrapped async audit logging in closure to explicitly discard error. Audit logging is fire-and-forget to avoid blocking sanitization.

Functions returning `(nil, nil)` are ambiguous - callers cannot distinguish between success-with-no-data and error states.

**Fixed:**

- `pkg/common/cluster/clusterutil.go` - Removed special handling of `ErrReserveFull`. Library code now reports all errors naturally using `%w` for proper error wrapping.
- `cmd/osde2e/provision/cmd.go` - Handles `ErrReserveFull` at application boundary where business logic belongs. Treats "reserve full" as success, all other errors propagate.
- `pkg/e2e/e2e.go` + `pkg/common/aws/s3.go` - Moved S3 bucket check to caller. Application layer decides whether to upload; library layer just uploads. Added TODO for future refactor to use
  dependency injection.
- `test/sdn_migration/sdn_migration_test.go` - Changed terraform destroy to return empty map instead of `(nil, nil)` to indicate success-with-no-output.

Ensures linter suppressions are properly formatted and prevents malformed directives.

**Fixed:**

- `pkg/common/providers/ocmprovider/cluster.go` - Fixed `//nolint:gocyclo` formatting (removed space).
- `pkg/common/concurrentviper/generated.go` - Removed malformed `// nolint` directive. File is already excluded via config since generator is broken.
- `cmd/osde2e/provision/cmd.go` - Removed unnecessary `//nolint:gocyclo` directive. Function complexity was reduced.

- **Clearer error contracts** - No more ambiguous (nil, nil) returns
- **Better separation of concerns** - Libraries report errors, applications interpret them
- **Explicit error handling** - All errors are handled or intentionally discarded
- **Type-safe error wrapping** - Using `%w` instead of `%v` for proper error chains

All changes maintain existing behavior while making error handling more explicit and maintainable.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* fix: add ReserveFull error logging back

---------

Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>

Author: christophermancini

.golangci.yml

@@ -6,16 +6,19 @@ linters:
   default: none
   enable:
     - copyloopvar
+    - errcheck
     - gocyclo
     - govet
     - ineffassign
     - loggercheck
     - mirror
     - misspell
+    - nilnil
+    - nolintlint
     - staticcheck
     - unconvert
-    - unused
     - unparam
+    - unused
   exclusions:
     generated: lax
     presets:
@@ -32,6 +35,7 @@ linters:
       - third_party$
       - builtin$
       - examples$
+      - pkg/common/concurrentviper/generated\.go$
 formatters:
   enable:
     - gofumpt

cmd/osde2e/provision/cmd.go

@@ -1,6 +1,7 @@
 package provision
 
 import (
+	"errors"
 	"fmt"
 
 	"github.com/openshift/osde2e/cmd/osde2e/common"
@@ -80,7 +81,6 @@ func init() {
 	_ = viper.BindPFlag(config.Tests.OnlyHealthCheckNodes, Cmd.PersistentFlags().Lookup("only-health-check-nodes"))
 }
 
-//nolint:gocyclo
 func run(cmd *cobra.Command, argv []string) error {
 	var err error
 	if err = common.LoadConfigs(args.configString, "", args.secretLocations); err != nil {
@@ -92,5 +92,8 @@ func run(cmd *cobra.Command, argv []string) error {
 	}
 
 	_, err = clusterutil.Provision(provider)
-	return err
+	if err != nil && !errors.Is(err, clusterutil.ErrReserveFull) {
+		return err
+	}
+	return nil
 }

internal/sanitizer/sanitizer.go

@@ -163,12 +163,14 @@ func (s *Sanitizer) sanitizeContent(content, source string, timestamp time.Time)
 
 	// Perform audit logging asynchronously to avoid blocking
 	if s.auditLog != nil && (matchCount > 0 || !s.config.SkipAuditOnNoMatch) {
-		go s.auditLog.Log(AuditEntry{
-			Timestamp:    timestamp,
-			Source:       source,
-			RulesApplied: rulesApplied,
-			MatchCount:   matchCount,
-		})
+		go func() {
+			_ = s.auditLog.Log(AuditEntry{
+				Timestamp:    timestamp,
+				Source:       source,
+				RulesApplied: rulesApplied,
+				MatchCount:   matchCount,
+			})
+		}()
 	}
 
 	return result, nil

pkg/common/aws/s3.go

@@ -140,12 +140,12 @@ type S3UploadResult struct {
 // Upload is automatically enabled when LOG_BUCKET is set.
 // Reuses the global CcsAwsSession for AWS credentials and session management.
 // The component parameter is used to organize artifacts in S3 (e.g., "osd-example-operator").
+//
+// TODO: Refactor to use dependency injection instead of viper globals.
+// Should accept (bucket, region, component string) parameters for better testability
+// and reusability. Caller should check config and decide whether to upload.
 func NewS3Uploader(component string) (*S3Uploader, error) {
 	bucket := viper.GetString(config.Tests.LogBucket)
-	if bucket == "" {
-		// S3 upload disabled - no bucket configured
-		return nil, nil
-	}
 
 	// Ensure region is set (default to us-east-1)
 	if viper.GetString(config.AWSRegion) == "" {

pkg/common/aws/s3_test.go

@@ -68,18 +68,6 @@ func TestBuildS3Key(t *testing.T) {
 	}
 }
 
-func TestNewS3Uploader_Disabled(t *testing.T) {
-	viper.Set(config.Tests.LogBucket, "")
-
-	uploader, err := NewS3Uploader("test-component")
-	if err != nil {
-		t.Errorf("NewS3Uploader() with disabled config returned error: %v", err)
-	}
-	if uploader != nil {
-		t.Error("NewS3Uploader() should return nil when LOG_BUCKET is empty")
-	}
-}
-
 func TestBuildBaseKey(t *testing.T) {
 	tests := []struct {
 		key      string

pkg/common/cluster/clusterutil.go

@@ -569,9 +569,8 @@ func Provision(provider spi.Provider) (*spi.Cluster, error) {
 	if err != nil {
 		if errors.Is(err, ErrReserveFull) {
 			log.Printf("Reserve full, exiting without provisioning")
-			return nil, nil
 		}
-		return nil, fmt.Errorf("failed to set up or retrieve cluster: %v", err)
+		return nil, fmt.Errorf("failed to set up or retrieve cluster: %w", err)
 	}
 
 	viper.Set(config.Cluster.ID, cluster.ID())

pkg/common/concurrentviper/generated.go

@@ -1,4 +1,3 @@
-// nolint
 package concurrentviper
 
 import (

pkg/common/providers/ocmprovider/cluster.go

@@ -64,7 +64,8 @@ func (o *OCMProvider) IsValidClusterName(clusterName string) (bool, error) {
 }
 
 // LaunchCluster setups an new cluster using the OSD API and returns it's ID.
-// nolint:gocyclo
+//
+//nolint:gocyclo
 func (o *OCMProvider) LaunchCluster(clusterName string) (string, error) {
 	flavourID := getFlavour()
 	skuID := getSKU()

pkg/e2e/e2e.go

@@ -249,27 +249,27 @@ func (o *E2EOrchestrator) Report(ctx context.Context) error {
 
 	runner.ReportClusterInstallLogs(o.provider)
 
-	// Upload test artifacts to S3 if bucket set
-	if viper.GetString(config.Tests.LogBucket) != "" {
-		if err := o.uploadToS3(); err != nil {
-			log.Printf("S3 upload failed: %v", err)
-			// Don't fail the overall report phase for S3 upload errors
-		}
+	// Upload test artifacts to S3 if bucket configured
+	if err := o.uploadToS3(); err != nil {
+		log.Printf("S3 upload failed: %v", err)
+		// Don't fail the overall report phase for S3 upload errors
 	}
 
 	return nil
 }
 
 // uploadToS3 uploads the report directory contents to S3.
 func (o *E2EOrchestrator) uploadToS3() error {
+	// Check if S3 bucket is configured
+	if viper.GetString(config.Tests.LogBucket) == "" {
+		return nil // S3 upload not configured, skip
+	}
+
 	component := deriveComponentFromTestImage()
 	uploader, err := aws.NewS3Uploader(component)
 	if err != nil {
 		return fmt.Errorf("failed to create S3 uploader: %w", err)
 	}
-	if uploader == nil {
-		return nil // S3 upload not enabled
-	}
 
 	reportDir := viper.GetString(config.ReportDir)
 	if reportDir == "" {

test/sdn_migration/sdn_migration_test.go

@@ -264,10 +264,10 @@ func runTerraformCommand(command string) (map[string]tfexec.OutputMeta, error) {
 		if err != nil {
 			return nil, fmt.Errorf("terraform destroy failed: %v", err.Error())
 		}
+		return map[string]tfexec.OutputMeta{}, nil
 	default:
 		return nil, fmt.Errorf("unknown command: %s", command)
 	}
-	return nil, nil
 }
 
 func cluterOperatorHealthCheck(ctx context.Context, clusterClient *openshiftclient.Client, logger logr.Logger) error {