diff --git a/.golangci.yml b/.golangci.yml index 980570c..6573e7f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,6 +1,8 @@ # golangci-lint configuration for HyperFleet Pull Secret Job # https://golangci-lint.run/usage/configuration/ +version: 2 + run: timeout: 5m tests: true @@ -8,22 +10,21 @@ run: linters: enable: - - gofmt # Checks whether code was gofmt-ed - - goimports # Checks import statements are formatted according to the 'goimport' command - govet # Reports suspicious constructs - errcheck # Checks for unchecked errors - staticcheck # Static analysis checks - unused # Checks for unused constants, variables, functions and types - - gosimple # Simplify code - ineffassign # Detects ineffectual assignments - - typecheck # Type-checks Go code - misspell # Finds commonly misspelled English words - revive # Fast, configurable, extensible, flexible, and beautiful linter for Go - gocritic # Provides diagnostics that check for bugs, performance and style issues +formatters: + enable: + - gofmt # Checks whether code was gofmt-ed + - goimports # Checks import statements are formatted according to the 'goimport' command + linters-settings: - gofmt: - simplify: true govet: enable: @@ -58,14 +59,9 @@ issues: linters: - errcheck - gosec - # Exclude pkg/job and pkg/config (external framework code) - - path: pkg/ - linters: - - revive - - goimports output: formats: - - format: colored-line-number + stdout: colored-line-number print-issued-lines: true print-linter-name: true diff --git a/Makefile b/Makefile index 5135d89..b570f3c 100644 --- a/Makefile +++ b/Makefile @@ -39,6 +39,7 @@ help: ## Display this help @echo "Build Targets:" @echo " make binary compile pull-secret binary" @echo " make test run unit tests with coverage" + @echo " make test-integration run integration tests" @echo " make lint run golangci-lint" @echo " make image build container image" @echo " make image-push build and push container image" @@ -48,6 +49,7 @@ help: ## Display this help @echo "Examples:" @echo " make binary" @echo " make test" + @echo " make test-integration" @echo " make lint" @echo " make image IMAGE_TAG=v1.0.0" @echo " make image-push IMAGE_TAG=v1.0.0" @@ -88,16 +90,31 @@ binary: check-gopath # Test & Lint Targets #################### -# Run unit tests with coverage +# Run unit tests with coverage (excludes test/ directory) test: - @echo "Running tests with coverage..." - go test -v -race -coverprofile=coverage.txt -covermode=atomic ./... + @echo "Running unit tests with coverage..." + go test -v -race -coverprofile=coverage.txt -covermode=atomic $$(go list ./... | grep -v '/test') @echo "" @echo "Coverage report generated: coverage.txt" @echo "View HTML coverage: go tool cover -html=coverage.txt" @echo "" .PHONY: test +# Run integration tests +test-integration: + @echo "Running integration tests..." + @if [ -n "$$(find ./test -name '*_test.go' 2>/dev/null)" ]; then \ + go test -v -race ./test/...; \ + echo ""; \ + echo "Integration tests complete."; \ + echo ""; \ + else \ + echo "No integration tests found in ./test/"; \ + echo "Create integration tests in ./test/ directory."; \ + echo ""; \ + fi +.PHONY: test-integration + # Run golangci-lint # Install: https://golangci-lint.run/usage/install/ lint: diff --git a/OWNERS b/OWNERS new file mode 100644 index 0000000..a57d2f2 --- /dev/null +++ b/OWNERS @@ -0,0 +1,33 @@ +approvers: +- "86254860" +- AlexVulaj +- aredenba-rh +- ciaranRoche +- crizzo71 +- jsell-rh +- mbrudnoy +- Mischulee +- rafabene +- rh-amarin +- tirthct +- vkareh +- xueli181114 +- yasun1 +- yingzhanredhat + +reviewers: +- "86254860" +- AlexVulaj +- aredenba-rh +- ciaranRoche +- crizzo71 +- jsell-rh +- mbrudnoy +- Mischulee +- rafabene +- rh-amarin +- tirthct +- vkareh +- xueli181114 +- yasun1 +- yingzhanredhat diff --git a/pkg/config/job.go b/pkg/config/job.go index 0f97611..7a0c0ac 100644 --- a/pkg/config/job.go +++ b/pkg/config/job.go @@ -1,12 +1,15 @@ +// Package config provides configuration types and utilities for job execution. package config import "github.com/spf13/pflag" +// JobConfig holds the configuration options for job execution. type JobConfig struct { DryRun bool `json:"dry_run"` WorkerCount int `json:"worker_count"` } +// NewJobConfig creates a new JobConfig with default values. func NewJobConfig() *JobConfig { return &JobConfig{ DryRun: true, @@ -14,6 +17,7 @@ func NewJobConfig() *JobConfig { } } +// AddFlags registers the job configuration flags with the provided flag set. func (c *JobConfig) AddFlags(fs *pflag.FlagSet) { fs.BoolVar(&c.DryRun, "dry-run", c.DryRun, "Show what would be changed by a run of this script.") fs.IntVar(&c.WorkerCount, "worker-count", c.WorkerCount, "Number of concurrent workers.") diff --git a/pkg/config/job_test.go b/pkg/config/job_test.go new file mode 100644 index 0000000..8fba72e --- /dev/null +++ b/pkg/config/job_test.go @@ -0,0 +1,64 @@ +package config + +import ( + "testing" + + "github.com/spf13/pflag" +) + +func TestNewJobConfig(t *testing.T) { + config := NewJobConfig() + + if config == nil { + t.Fatal("NewJobConfig() returned nil") + } + + if !config.DryRun { + t.Errorf("Expected DryRun to be true by default, got false") + } + + if config.WorkerCount != 1 { + t.Errorf("Expected WorkerCount to be 1 by default, got %d", config.WorkerCount) + } +} + +func TestJobConfig_AddFlags(t *testing.T) { + config := NewJobConfig() + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + + config.AddFlags(fs) + + // Verify flags were added + if fs.Lookup("dry-run") == nil { + t.Error("Expected 'dry-run' flag to be registered") + } + + if fs.Lookup("worker-count") == nil { + t.Error("Expected 'worker-count' flag to be registered") + } + + // TODO: Add more comprehensive flag parsing tests +} + +func TestJobConfig_FlagParsing(t *testing.T) { + config := NewJobConfig() + fs := pflag.NewFlagSet("test", pflag.ContinueOnError) + + config.AddFlags(fs) + + // Test parsing custom values + args := []string{"--dry-run=false", "--worker-count=5"} + if err := fs.Parse(args); err != nil { + t.Fatalf("Failed to parse flags: %v", err) + } + + if config.DryRun { + t.Errorf("Expected DryRun to be false after parsing, got true") + } + + if config.WorkerCount != 5 { + t.Errorf("Expected WorkerCount to be 5 after parsing, got %d", config.WorkerCount) + } + + // TODO: Add edge case tests (negative worker count, etc.) +} diff --git a/pkg/job/job.go b/pkg/job/job.go index eff1ba6..c5e0043 100644 --- a/pkg/job/job.go +++ b/pkg/job/job.go @@ -1,3 +1,4 @@ +// Package job provides a framework for defining and executing concurrent jobs with task queues and worker pools. package job import ( @@ -43,39 +44,47 @@ type CommandBuilder struct { // panicHandler is an optional function that accepts interface and can deal with it how it wants. // An example use can be to capture any errors and report it to sentry. Not setting panicHandler means any panics // encountered will be silently recovered. - panicHandler func(ctx context.Context, any interface{}) + panicHandler func(ctx context.Context, panicValue interface{}) metricsReporter MetricsReporter } +// SetRegistry sets the job registry for the command builder. func (b *CommandBuilder) SetRegistry(registry JobRegistry) *CommandBuilder { b.registry = registry return b } +// SetContext sets the context for the command builder. func (b *CommandBuilder) SetContext(ctx context.Context) *CommandBuilder { b.ctx = ctx return b } +// SetBeforeJob sets the hook function to execute before job execution. func (b *CommandBuilder) SetBeforeJob(fn func(ctx context.Context) error) *CommandBuilder { b.beforeJob = fn return b } + +// SetAfterJob sets the hook function to execute after job execution. func (b *CommandBuilder) SetAfterJob(fn func(ctx context.Context)) *CommandBuilder { b.afterJob = fn return b } -func (b *CommandBuilder) SetPanicHandler(fn func(ctx context.Context, any interface{})) *CommandBuilder { +// SetPanicHandler sets the panic handler function for the command builder. +func (b *CommandBuilder) SetPanicHandler(fn func(ctx context.Context, panicValue interface{})) *CommandBuilder { b.panicHandler = fn return b } +// SetMetricsReporter sets the metrics reporter for the command builder. func (b *CommandBuilder) SetMetricsReporter(reporter MetricsReporter) *CommandBuilder { b.metricsReporter = reporter return b } +// Build creates and returns a Cobra command with all registered jobs as subcommands. func (b *CommandBuilder) Build() *cobra.Command { cmd := &cobra.Command{ Use: "run-job", @@ -89,7 +98,7 @@ func (b *CommandBuilder) Build() *cobra.Command { Long: job.GetMetadata().Description, // We don't need this info if job fails. SilenceUsage: true, - RunE: func(cmd *cobra.Command, args []string) error { + RunE: func(_ *cobra.Command, _ []string) error { err := validateJob(job) if err != nil { return err @@ -118,14 +127,19 @@ func validateJob(job Job) error { return nil } +// JobRegistry maintains a collection of jobs that can be registered and executed. +// +//nolint:revive // JobRegistry is preferred over Registry for clarity in external usage type JobRegistry struct { jobs []Job } +// NewJobRegistry creates a new job registry. func NewJobRegistry() *JobRegistry { return &JobRegistry{} } +// AddJob adds a job to the registry. func (r *JobRegistry) AddJob(job Job) { if job == nil { return @@ -171,7 +185,7 @@ func newTaskQueue() *taskQueue { type workerPool struct { Queue *taskQueue Workers int - PanicHandler func(ctx context.Context, any interface{}) + PanicHandler func(ctx context.Context, panicValue interface{}) MetricsCollector *MetricsCollector } @@ -192,11 +206,11 @@ func (wp *workerPool) Run(ctx context.Context) { return } func() { - taskId := ksuid.New().String() + taskID := ksuid.New().String() taskCtx := AddTraceContext(ctx, "workerId", strconv.Itoa(workerId)) taskCtx = AddTraceContext(taskCtx, "taskName", task.TaskName()) - taskCtx = AddTraceContext(taskCtx, "taskId", taskId) + taskCtx = AddTraceContext(taskCtx, "taskId", taskID) defer func(taskCtx context.Context) { if err := recover(); err != nil { @@ -208,7 +222,7 @@ func (wp *workerPool) Run(ctx context.Context) { } }(taskCtx) - logger.NewOCMLogger(taskCtx).Contextual().Info("Processing task", "workerId", workerId, "taskId", taskId) + logger.NewOCMLogger(taskCtx).Contextual().Info("Processing task", "workerId", workerId, "taskId", taskID) err := task.Process(taskCtx) if err != nil { wp.MetricsCollector.IncTaskFailed() @@ -235,7 +249,7 @@ var _ runner = &TestRunner{} type jobRunner struct { BeforeJob func(ctx context.Context) error AfterJob func(ctx context.Context) - PanicHandler func(ctx context.Context, any interface{}) + PanicHandler func(ctx context.Context, panicValue interface{}) MetricsReporter MetricsReporter } @@ -283,7 +297,7 @@ func (jr jobRunner) Run(ctx context.Context, job Job, workerCount int) error { } for _, task := range tasks { taskQueue.Add(task) - taskTotal += 1 + taskTotal++ } metricsCollector := NewMetricsCollector(job.GetMetadata().Use) metricsCollector.SetTaskTotal(uint32(taskTotal)) @@ -320,6 +334,7 @@ func (jr jobRunner) Run(ctx context.Context, job Job, workerCount int) error { // TestRunner is a lightweight JobRunner implementation to enable for easy testing of job logic. type TestRunner struct{} +// Run executes the job in test mode without lifecycle hooks. func (tr TestRunner) Run(ctx context.Context, job Job, workerCount int) error { taskTotal := 0 taskQueue := newTaskQueue() @@ -331,7 +346,7 @@ func (tr TestRunner) Run(ctx context.Context, job Job, workerCount int) error { } for _, task := range tasks { taskQueue.Add(task) - taskTotal += 1 + taskTotal++ } metricsCollector := NewMetricsCollector(job.GetMetadata().Use) metricsCollector.SetTaskTotal(uint32(taskTotal)) @@ -339,6 +354,11 @@ func (tr TestRunner) Run(ctx context.Context, job Job, workerCount int) error { pool := workerPool{Queue: taskQueue, Workers: workerCount, PanicHandler: nil, MetricsCollector: metricsCollector} pool.Run(ctx) + if metricsCollector.taskTotal == 0 { + // No tasks to run + return nil + } + // For now return error when all tasks fail. This can be configurable for e.g. return error when 80% of tasks fail. if metricsCollector.taskFailed == metricsCollector.taskTotal { err := errors.New("all tasks failed") return err diff --git a/pkg/job/metrics.go b/pkg/job/metrics.go index 4c19647..7540e62 100644 --- a/pkg/job/metrics.go +++ b/pkg/job/metrics.go @@ -7,6 +7,7 @@ import ( logger "github.com/openshift-online/ocm-service-common/pkg/ocmlogger" ) +// MetricsReporter defines the interface for reporting metrics collected during job execution. type MetricsReporter interface { Report(metricsCollector *MetricsCollector) } @@ -21,24 +22,31 @@ type MetricsCollector struct { taskFailed uint32 } +// NewMetricsCollector creates a new metrics collector for the given job name. func NewMetricsCollector(jobName string) *MetricsCollector { return &MetricsCollector{jobName: jobName} } +// SetTaskTotal sets the total number of tasks. func (m *MetricsCollector) SetTaskTotal(total uint32) { m.taskTotal = total } + +// IncTaskSuccess increments the successful task counter. func (m *MetricsCollector) IncTaskSuccess() { m.mu.Lock() m.taskSuccess++ m.mu.Unlock() } + +// IncTaskFailed increments the failed task counter. func (m *MetricsCollector) IncTaskFailed() { m.mu.Lock() m.taskFailed++ m.mu.Unlock() } +// Snapshot returns a point-in-time copy of the metrics collector. func (m *MetricsCollector) Snapshot() MetricsCollector { m.mu.Lock() defer m.mu.Unlock() @@ -52,15 +60,18 @@ func (m *MetricsCollector) Snapshot() MetricsCollector { } +// StdoutReporter reports metrics to stdout. type StdoutReporter struct { } +// Report prints the metrics to stdout using the logger. 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) } +// NewStdoutReporter creates a new stdout metrics reporter. func NewStdoutReporter() MetricsReporter { return StdoutReporter{} } diff --git a/pkg/job/trace_context.go b/pkg/job/trace_context.go index d3c81ab..3159185 100644 --- a/pkg/job/trace_context.go +++ b/pkg/job/trace_context.go @@ -2,8 +2,9 @@ package job import ( "context" - logger "github.com/openshift-online/ocm-service-common/pkg/ocmlogger" "sync" + + logger "github.com/openshift-online/ocm-service-common/pkg/ocmlogger" ) type ( @@ -23,6 +24,8 @@ var ( // so user does not need to worry about callback registration separately. // The implementation is thread-safe to prevent concurrent access to `set` and `logger.RegisterExtraDataCallback`. +// AddTraceContext adds a key-value pair to the context for tracing and logging purposes. +// It registers a callback for the key on first use and returns the enriched context. func AddTraceContext(ctx context.Context, key string, value string) context.Context { lock.Lock() defer lock.Unlock()