diff --git a/.golangci.yml b/.golangci.yml index 6924b56..ff1a708 100644 --- a/.golangci.yml +++ b/.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,7 +11,8 @@ 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) @@ -18,16 +20,13 @@ linters: - 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 diff --git a/cmd/pull-secret/jobs/pull_secret.go b/cmd/pull-secret/jobs/pull_secret.go index 1a6d16e..5abae7a 100644 --- a/cmd/pull-secret/jobs/pull_secret.go +++ b/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,35 +109,44 @@ 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 } @@ -144,7 +154,8 @@ func (e PullSecretTask) Process(ctx context.Context) error { 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 } @@ -152,7 +163,8 @@ func (e PullSecretTask) Process(ctx context.Context) error { 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<"), fmt.Sprintf("[Task %s] Panic", task.TaskName()), "exception", err) + logger.NewOCMLogger(taskCtx).Contextual().Error( + fmt.Errorf(""), + 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(""), 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(""), + 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,11 +299,17 @@ 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 { @@ -298,8 +317,9 @@ func (jr jobRunner) Run(ctx context.Context, job Job, workerCount int) error { 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} diff --git a/pkg/job/metrics.go b/pkg/job/metrics.go index 4c19647..8a47260 100644 --- a/pkg/job/metrics.go +++ b/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 {