From e2e792f900b3af4554b042641f79856fac2150b1 Mon Sep 17 00:00:00 2001 From: Tongtong Zhou Date: Fri, 9 Jan 2026 11:56:23 +0800 Subject: [PATCH 1/2] Fix lint issues and enable 15/16 HyperFleet standard linters --- .golangci.yml | 47 +++++++------------ cmd/pull-secret/jobs/pull_secret.go | 58 ++++++++++++++++-------- cmd/pull-secret/jobs/pull_secret_test.go | 4 +- cmd/pull-secret/main.go | 3 +- pkg/job/job.go | 37 +++++++++++---- pkg/job/metrics.go | 6 ++- 6 files changed, 94 insertions(+), 61 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 6924b56..a49a63b 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) +# All 16 required linters enabled per HyperFleet standard 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 { From 0bd9f8313d815e2efc83b89860d63ca0756bab19 Mon Sep 17 00:00:00 2001 From: Tongtong Zhou Date: Fri, 9 Jan 2026 12:18:06 +0800 Subject: [PATCH 2/2] Address CodeRabbit review feedback --- .golangci.yml | 2 +- cmd/pull-secret/jobs/pull_secret_test.go | 10 +++------- cmd/pull-secret/main.go | 3 +-- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index a49a63b..ff1a708 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,7 +1,7 @@ # golangci-lint configuration for HyperFleet Pull Secret Job # https://golangci-lint.run/usage/configuration/ # Aligned with HyperFleet Linting Standard (HYPERFLEET-443) -# All 16 required linters enabled per HyperFleet standard +# 15 of 16 required linters enabled (revive temporarily disabled) version: "2" run: diff --git a/cmd/pull-secret/jobs/pull_secret_test.go b/cmd/pull-secret/jobs/pull_secret_test.go index ee6bb66..3064720 100644 --- a/cmd/pull-secret/jobs/pull_secret_test.go +++ b/cmd/pull-secret/jobs/pull_secret_test.go @@ -2,7 +2,6 @@ package jobs import ( "context" - "os" "testing" "google.golang.org/grpc/codes" @@ -267,14 +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 { - if err := os.Setenv(k, v); err != nil { - t.Fatalf("failed to set env var %s: %v", k, err) - } + t.Setenv(k, v) } - defer os.Clearenv() job := &PullSecretJob{} tasks, err := job.GetTasks() diff --git a/cmd/pull-secret/main.go b/cmd/pull-secret/main.go index 0ca49aa..2a39133 100644 --- a/cmd/pull-secret/main.go +++ b/cmd/pull-secret/main.go @@ -20,8 +20,7 @@ import ( func init() { logger.SetTrimList([]string{"pull-secret", "pkg"}) - //nolint:errcheck // Default log level, error can be safely ignored - _ = logger.SetLogLevel(logger.OCM_LOG_LEVEL_DEFAULT) + _ = logger.SetLogLevel(logger.OCM_LOG_LEVEL_DEFAULT) //nolint:errcheck // safe to ignore } func main() {