HYPERFLEET-443 | Fix lint issues and enable 15/16 HyperFleet standard linters

.golangci.yml

@@ -1,6 +1,7 @@
 # golangci-lint configuration for HyperFleet Pull Secret Job
 # https://golangci-lint.run/usage/configuration/
 # Aligned with HyperFleet Linting Standard (HYPERFLEET-443)
+# 15 of 16 required linters enabled (revive temporarily disabled)
 version: "2"
 
 run:
@@ -10,24 +11,22 @@ run:
 
 linters:
   enable:
-    # Required linters per HyperFleet standard
+    # HyperFleet standard linters (15 of 16 enabled)
+    # TODO: Re-enable revive after adding comments to exported symbols
     - govet          # Reports suspicious constructs
     - errcheck       # Checks for unchecked errors
     - staticcheck    # Static analysis checks (includes gosimple in v2)
     - unused         # Checks for unused constants, variables, functions and types
     - ineffassign    # Detects ineffectual assignments
     - misspell       # Finds commonly misspelled English words
     - gocritic       # Provides diagnostics that check for bugs, performance and style issues
-    # Additional linters per HYPERFLEET-443
     - unconvert      # Remove unnecessary type conversions
     - unparam        # Reports unused function parameters
     - goconst        # Finds repeated strings that could be replaced by a constant
     - exhaustive     # Check exhaustiveness of enum switch statements
-    # Temporarily disabled - existing code has violations, fix incrementally
-    # TODO: Enable these linters after fixing existing violations
-    # - revive       # Fast linter (needs comment fixes first)
-    # - lll          # Reports long lines
-    # - gosec        # Inspects source code for security problems
+    # - revive       # Temporarily disabled - 44 existing violations (missing comments)
+    - lll            # Reports long lines
+    - gosec          # Inspects source code for security problems
 
 linters-settings:
   gofmt:
@@ -42,21 +41,24 @@ linters-settings:
     check-blank: true
 
   revive:
+    enable-all-rules: false
     rules:
+      # Explicitly disable all rules with existing violations
+      # TODO: Enable incrementally as comments are added
       - name: exported
-        disabled: true  # Disabled for MVP - can enable later
+        disabled: true
       - name: unexported-return
-        disabled: true  # Disabled for MVP
+        disabled: true
       - name: var-naming
-        disabled: true  # Disabled for MVP
+        disabled: true
       - name: package-comments
-        disabled: true  # Disabled for MVP
+        disabled: true
       - name: redefines-builtin-id
-        disabled: true  # Disabled for MVP
+        disabled: true
       - name: unused-parameter
-        disabled: true  # Disabled for MVP
+        disabled: true
       - name: increment-decrement
-        disabled: true  # Disabled for MVP
+        disabled: true
 
   # Settings per HyperFleet standard
   lll:
@@ -74,26 +76,9 @@ issues:
   max-issues-per-linter: 0
   max-same-issues: 0
 
-  # Exclude some linters from running on tests files
-  exclude-rules:
-    - path: _test\.go
-      linters:
-        - errcheck
-        - gosec
-        - lll
-    # Temporarily exclude existing violations in pkg/ for incremental fixes
-    # TODO: Remove these exclusions as issues are fixed
-    - path: "^pkg/"
-      linters:
-        - revive      # Missing comments - fix incrementally
-        - lll         # Long lines - fix incrementally
-        - gosec       # G115 integer overflow - needs careful review
-        - goimports   # Formatting issues
-
 formatters:
   enable:
     - gofmt
-    # - goimports  # TODO: Enable after fixing existing formatting issues
 
 output:
   print-issued-lines: true

cmd/pull-secret/jobs/pull_secret.go

@@ -18,7 +18,7 @@ import (
 )
 
 const (
-	pullSecretTaskName = "pull-secret-mvp"
+	pullSecretTaskName = "pull-secret-mvp" //nolint:gosec // G101: false positive, not a credential
 )
 
 type PullSecretTask struct {
@@ -91,7 +91,8 @@ func (pullsecretJob *PullSecretJob) GetMetadata() job.Metadata {
 }
 
 func (pullsecretJob *PullSecretJob) AddFlags(flags *pflag.FlagSet) {
-	flags.BoolVar(&pullsecretJob.DryRun, "dry-run", false, "Dry run mode - validate authentication and configuration without creating/updating secrets")
+	flags.BoolVar(&pullsecretJob.DryRun, "dry-run", false,
+		"Dry run mode - validate authentication and configuration without creating/updating secrets")
 }
 
 func (pullsecretJob *PullSecretJob) GetWorkerCount() int {
@@ -108,51 +109,62 @@ func (e PullSecretTask) Process(ctx context.Context) error {
 
 	// Validate pull secret JSON format
 	if err := validatePullSecret(e.PullSecret); err != nil {
-		logStructured("error", e.ClusterID, e.GCPProjectID, "validate-pull-secret", 0, fmt.Sprintf("Invalid pull secret format: %v", err), "")
+		logStructured("error", e.ClusterID, e.GCPProjectID, "validate-pull-secret", 0,
+			fmt.Sprintf("Invalid pull secret format: %v", err), "")
 		return fmt.Errorf("invalid pull secret format: %w", err)
 	}
 
 	if e.DryRun {
-		logStructured("info", e.ClusterID, e.GCPProjectID, "start", 0, "Starting pull secret storage operation (DRY RUN MODE)", "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "start", 0,
+			"Starting pull secret storage operation (DRY RUN MODE)", "")
 	} else {
-		logStructured("info", e.ClusterID, e.GCPProjectID, "start", 0, "Starting pull secret storage operation", "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "start", 0,
+			"Starting pull secret storage operation", "")
 	}
 
 	// Initialize Secret Manager client
 	client, err := secretmanager.NewClient(ctx)
 	if err != nil {
-		logStructured("error", e.ClusterID, e.GCPProjectID, "init-client", 0, fmt.Sprintf("Failed to create secretmanager client: %v", err), "")
+		logStructured("error", e.ClusterID, e.GCPProjectID, "init-client", 0,
+			fmt.Sprintf("Failed to create secretmanager client: %v", err), "")
 		return fmt.Errorf("failed to create secretmanager client: %w", err)
 	}
 	defer func() {
 		if closeErr := client.Close(); closeErr != nil {
-			logStructured("error", e.ClusterID, e.GCPProjectID, "close-client", 0, fmt.Sprintf("Failed to close client: %v", closeErr), "")
+			logStructured("error", e.ClusterID, e.GCPProjectID, "close-client", 0,
+				fmt.Sprintf("Failed to close client: %v", closeErr), "")
 		}
 	}()
 
-	logStructured("info", e.ClusterID, e.GCPProjectID, "client-initialized", 0, "Successfully initialized Secret Manager client", "")
+	logStructured("info", e.ClusterID, e.GCPProjectID, "client-initialized", 0,
+		"Successfully initialized Secret Manager client", "")
 
 	// In dry-run mode, skip actual secret operations
 	if e.DryRun {
-		logStructured("info", e.ClusterID, e.GCPProjectID, "dry-run", 0, "DRY RUN: Skipping secret creation/update operations", "")
-		logStructured("info", e.ClusterID, e.GCPProjectID, "dry-run", 0, fmt.Sprintf("DRY RUN: Would create/update secret: %s", e.SecretName), "")
-		logStructured("info", e.ClusterID, e.GCPProjectID, "completed", 0, "DRY RUN completed successfully - authentication validated", "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "dry-run", 0,
+			"DRY RUN: Skipping secret creation/update operations", "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "dry-run", 0,
+			fmt.Sprintf("DRY RUN: Would create/update secret: %s", e.SecretName), "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "completed", 0,
+			"DRY RUN completed successfully - authentication validated", "")
 		return nil
 	}
 
 	// Create or update secret with retry logic
 	if err := retryWithBackoff(ctx, func() error {
 		return e.createOrUpdateSecret(ctx, client)
 	}, 3); err != nil {
-		logStructured("error", e.ClusterID, e.GCPProjectID, "create-update-secret", 0, fmt.Sprintf("Failed to create/update secret: %v", err), "")
+		logStructured("error", e.ClusterID, e.GCPProjectID, "create-update-secret", 0,
+			fmt.Sprintf("Failed to create/update secret: %v", err), "")
 		return err
 	}
 
 	// Verify secret is accessible
 	if err := retryWithBackoff(ctx, func() error {
 		return e.verifySecret(ctx, client)
 	}, 3); err != nil {
-		logStructured("error", e.ClusterID, e.GCPProjectID, "verify-secret", 0, fmt.Sprintf("Failed to verify secret: %v", err), "")
+		logStructured("error", e.ClusterID, e.GCPProjectID, "verify-secret", 0,
+			fmt.Sprintf("Failed to verify secret: %v", err), "")
 		return err
 	}
 
@@ -190,25 +202,30 @@ func (e PullSecretTask) createOrUpdateSecret(ctx context.Context, client *secret
 
 	if !exists {
 		// Create new secret
-		logStructured("info", e.ClusterID, e.GCPProjectID, "create-secret", 0, fmt.Sprintf("Creating new secret: %s", e.SecretName), "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "create-secret", 0,
+			fmt.Sprintf("Creating new secret: %s", e.SecretName), "")
 		if createErr := e.createSecret(ctx, client); createErr != nil {
 			return fmt.Errorf("failed to create secret: %w", createErr)
 		}
 		duration := time.Since(startTime).Milliseconds()
-		logStructured("info", e.ClusterID, e.GCPProjectID, "create-secret", duration, "Successfully created secret", "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "create-secret", duration,
+			"Successfully created secret", "")
 	} else {
-		logStructured("info", e.ClusterID, e.GCPProjectID, "secret-exists", 0, fmt.Sprintf("Secret already exists: %s", e.SecretName), "")
+		logStructured("info", e.ClusterID, e.GCPProjectID, "secret-exists", 0,
+			fmt.Sprintf("Secret already exists: %s", e.SecretName), "")
 	}
 
 	// Add secret version with data
 	startTime = time.Now()
-	logStructured("info", e.ClusterID, e.GCPProjectID, "add-secret-version", 0, "Adding secret version with pull secret data", "")
+	logStructured("info", e.ClusterID, e.GCPProjectID, "add-secret-version", 0,
+		"Adding secret version with pull secret data", "")
 	version, err := e.addSecretVersion(ctx, client)
 	if err != nil {
 		return fmt.Errorf("failed to add secret version: %w", err)
 	}
 	duration := time.Since(startTime).Milliseconds()
-	logStructured("info", e.ClusterID, e.GCPProjectID, "add-secret-version", duration, "Successfully created secret version", version)
+	logStructured("info", e.ClusterID, e.GCPProjectID, "add-secret-version", duration,
+		"Successfully created secret version", version)
 
 	return nil
 }
@@ -290,7 +307,8 @@ func (e PullSecretTask) verifySecret(ctx context.Context, client *secretmanager.
 	}
 
 	duration := time.Since(startTime).Milliseconds()
-	logStructured("info", e.ClusterID, e.GCPProjectID, "verify-secret", duration, fmt.Sprintf("Verified secret (%d bytes)", len(result.Payload.Data)), "")
+	logStructured("info", e.ClusterID, e.GCPProjectID, "verify-secret", duration,
+		fmt.Sprintf("Verified secret (%d bytes)", len(result.Payload.Data)), "")
 
 	return nil
 }
@@ -330,9 +348,11 @@ func retryWithBackoff(ctx context.Context, fn func() error, maxRetries int) erro
 
 		if i < maxRetries-1 {
 			// Calculate backoff with jitter (±20%)
+			//nolint:gosec // G115: i is bounded by maxRetries (small value)
 			baseBackoff := time.Duration(1<<uint(i)) * time.Second
 			jitterRange := float64(baseBackoff) * 0.2
 			// Random value between -20% and +20% of base backoff
+			//nolint:gosec // G404: weak random is acceptable for jitter calculation
 			jitter := time.Duration((rand.Float64()*2 - 1) * jitterRange)
 			backoff := baseBackoff + jitter
 

cmd/pull-secret/jobs/pull_secret_test.go

@@ -2,7 +2,6 @@ package jobs
 
 import (
 	"context"
-	"os"
 	"testing"
 
 	"google.golang.org/grpc/codes"
@@ -267,12 +266,11 @@ func TestPullSecretJob_GetTasks(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			// Clear and set environment variables
-			os.Clearenv()
+			// Set environment variables using t.Setenv for proper test isolation
+			// t.Setenv automatically restores the original value after the test
 			for k, v := range tt.envVars {
-				os.Setenv(k, v)
+				t.Setenv(k, v)
 			}
-			defer os.Clearenv()
 
 			job := &PullSecretJob{}
 			tasks, err := job.GetTasks()

cmd/pull-secret/main.go

@@ -20,7 +20,7 @@ import (
 
 func init() {
 	logger.SetTrimList([]string{"pull-secret", "pkg"})
-	_ = logger.SetLogLevel(logger.OCM_LOG_LEVEL_DEFAULT) //nolint:errcheck // Default log level, error can be safely ignored
+	_ = logger.SetLogLevel(logger.OCM_LOG_LEVEL_DEFAULT) //nolint:errcheck // safe to ignore
 }
 
 func main() {

pkg/job/job.go

@@ -97,7 +97,13 @@ func (b *CommandBuilder) Build() *cobra.Command {
 				if b.metricsReporter == nil {
 					b.metricsReporter = NewStdoutReporter()
 				}
-				err = jobRunner{BeforeJob: b.beforeJob, AfterJob: b.afterJob, PanicHandler: b.panicHandler, MetricsReporter: b.metricsReporter}.Run(b.ctx, job, job.GetWorkerCount())
+				runner := jobRunner{
+					BeforeJob:       b.beforeJob,
+					AfterJob:        b.afterJob,
+					PanicHandler:    b.panicHandler,
+					MetricsReporter: b.metricsReporter,
+				}
+				err = runner.Run(b.ctx, job, job.GetWorkerCount())
 				if err != nil {
 					return err
 				}
@@ -201,7 +207,10 @@ func (wp *workerPool) Run(ctx context.Context) {
 					defer func(taskCtx context.Context) {
 						if err := recover(); err != nil {
 							wp.MetricsCollector.IncTaskFailed()
-							logger.NewOCMLogger(taskCtx).Contextual().Error(fmt.Errorf("<panic summary should go here>"), fmt.Sprintf("[Task %s] Panic", task.TaskName()), "exception", err)
+							logger.NewOCMLogger(taskCtx).Contextual().Error(
+								fmt.Errorf("<panic summary should go here>"),
+								fmt.Sprintf("[Task %s] Panic", task.TaskName()),
+								"exception", err)
 							if wp.PanicHandler != nil {
 								wp.PanicHandler(taskCtx, err)
 							}
@@ -241,7 +250,8 @@ type jobRunner struct {
 
 // Run executes the given job using a worker pool.
 //
-// It first invokes the BeforeJob hook (if defined). Then, it enqueues all tasks and delegates to worker pool for execution.
+// It first invokes the BeforeJob hook (if defined). Then, it enqueues all tasks
+// and delegates to worker pool for execution.
 // After all tasks are processed, the AfterJob hook is called.
 func (jr jobRunner) Run(ctx context.Context, job Job, workerCount int) error {
 	ctx = AddTraceContext(ctx, "jobName", job.GetMetadata().Use)
@@ -255,8 +265,11 @@ func (jr jobRunner) Run(ctx context.Context, job Job, workerCount int) error {
 				jr.PanicHandler(ctx, err)
 			}
 
-			ulog.Contextual().Error(fmt.Errorf("<panic summary should go here>"), fmt.Sprintf("[Job %s] Panic", job.GetMetadata().Use), "exception", err)
-			// We are purposefully re-throwing panic in main goroutine, so that job fails, and we don't suppress any errors.
+			ulog.Contextual().Error(
+				fmt.Errorf("<panic summary should go here>"),
+				fmt.Sprintf("[Job %s] Panic", job.GetMetadata().Use),
+				"exception", err)
+			// We are purposefully re-throwing panic in main goroutine
 			// This is opposite of how we handle panic in worker pool, where we need to handle any panics from individual tasks
 			// so we can protect other workers from executing.
 			panic(err)
@@ -286,20 +299,27 @@ func (jr jobRunner) Run(ctx context.Context, job Job, workerCount int) error {
 		taskTotal += 1
 	}
 	metricsCollector := NewMetricsCollector(job.GetMetadata().Use)
+	//nolint:gosec // G115: taskTotal is bounded by number of tasks (small value)
 	metricsCollector.SetTaskTotal(uint32(taskTotal))
 
 	ulog.Contextual().Info("queued all the tasks")
 
-	pool := workerPool{Queue: taskQueue, Workers: workerCount, PanicHandler: jr.PanicHandler, MetricsCollector: metricsCollector}
+	pool := workerPool{
+		Queue:            taskQueue,
+		Workers:          workerCount,
+		PanicHandler:     jr.PanicHandler,
+		MetricsCollector: metricsCollector,
+	}
 	pool.Run(ctx)
 
 	if jr.AfterJob != nil {
 		ulog.Contextual().Info("executing afterJob hook")
 		jr.AfterJob(ctx)
 	}
 
-	// For now, we report metrics only once at the end. In the future, we may need to support periodic reporting or
-	// synchronous updates (e.g., when counters are modified) to integrate with push-based systems like Prometheus Pushgateway.
+	// For now, we report metrics only once at the end. In the future, we may need
+	// to support periodic reporting or synchronous updates (e.g., when counters
+	// are modified) to integrate with push-based systems like Prometheus Pushgateway.
 	jr.MetricsReporter.Report(metricsCollector)
 
 	if metricsCollector.taskTotal == 0 {
@@ -334,6 +354,7 @@ func (tr TestRunner) Run(ctx context.Context, job Job, workerCount int) error {
 		taskTotal += 1
 	}
 	metricsCollector := NewMetricsCollector(job.GetMetadata().Use)
+	//nolint:gosec // G115: taskTotal is bounded by number of tasks (small value)
 	metricsCollector.SetTaskTotal(uint32(taskTotal))
 
 	pool := workerPool{Queue: taskQueue, Workers: workerCount, PanicHandler: nil, MetricsCollector: metricsCollector}

pkg/job/metrics.go

@@ -58,7 +58,11 @@ type StdoutReporter struct {
 func (r StdoutReporter) Report(metricsCollector *MetricsCollector) {
 	// use snapshot for point-in-time data
 	snapshot := metricsCollector.Snapshot()
-	logger.NewOCMLogger(context.Background()).Contextual().Info("Printing metrics to STDOUT", "task_total", snapshot.taskTotal, "task_success", snapshot.taskSuccess, "task_failed", snapshot.taskFailed)
+	logger.NewOCMLogger(context.Background()).Contextual().Info(
+		"Printing metrics to STDOUT",
+		"task_total", snapshot.taskTotal,
+		"task_success", snapshot.taskSuccess,
+		"task_failed", snapshot.taskFailed)
 }
 
 func NewStdoutReporter() MetricsReporter {