Fix the failure for lint/unit/integration#3
Fix the failure for lint/unit/integration#3yingzhanredhat wants to merge 1 commit intoopenshift-hyperfleet:mainfrom
Conversation
WalkthroughThis PR introduces a job execution framework and configuration system. It adds a Sequence DiagramsequenceDiagram
participant User
participant CommandBuilder as CommandBuilder<br/>(Setup)
participant JobRegistry as JobRegistry
participant Job as Job<br/>(Execution)
participant Metrics as MetricsCollector
participant Hooks as Lifecycle<br/>Hooks
User->>CommandBuilder: SetRegistry(registry)
User->>CommandBuilder: SetBeforeJob(fn)
User->>CommandBuilder: SetAfterJob(fn)
User->>CommandBuilder: SetMetricsReporter(reporter)
User->>JobRegistry: AddJob(job)
User->>CommandBuilder: Build() & Execute
CommandBuilder->>Hooks: BeforeJob(ctx)
Hooks-->>CommandBuilder: callback
CommandBuilder->>Job: Run with TaskID<br/>from Registry
Job->>Metrics: SetTaskTotal(count)
Job->>Job: Execute Tasks
Job->>Metrics: IncTaskSuccess()
Job->>Metrics: IncTaskFailed()
Job-->>CommandBuilder: completed
CommandBuilder->>Hooks: AfterJob(ctx)
Hooks-->>CommandBuilder: callback
CommandBuilder->>Metrics: Snapshot()
Metrics-->>CommandBuilder: report
CommandBuilder-->>User: execution result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Key areas requiring attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Makefile (1)
73-87: Pipeline failure:cmd/pull-secretdirectory does not exist.The CI pipeline failed because the
binarytarget references./cmd/pull-secretwhich doesn't exist in the repository. This PR should either:
- Add the missing
cmd/pull-secretdirectory with the main package, or- Update the Makefile to reference the correct path
#!/bin/bash # Verify the cmd directory structure echo "Checking for cmd directories:" fd -t d 'cmd' --max-depth 2 echo "" echo "Checking for main.go files:" fd 'main.go'pkg/job/job.go (1)
319-319: Add getter methods to avoid breaking encapsulation.Direct access to unexported
metricsCollector.taskTotalandmetricsCollector.taskFailedfields bypasses theMetricsCollector's thread-safety design. While safe in the current sequential access pattern (afterpool.Runcompletes), this violates encapsulation and makes the code fragile to future changes.🔎 Add getter methods to MetricsCollector:
In
pkg/job/metrics.go, add these getter methods:// GetTaskTotal returns the total number of tasks. func (m *MetricsCollector) GetTaskTotal() uint32 { m.mu.Lock() defer m.mu.Unlock() return m.taskTotal } // GetTaskFailed returns the number of failed tasks. func (m *MetricsCollector) GetTaskFailed() uint32 { m.mu.Lock() defer m.mu.Unlock() return m.taskFailed }Then update the access in
pkg/job/job.go:- if metricsCollector.taskTotal == 0 { + if metricsCollector.GetTaskTotal() == 0 { // this can happen when there are no tasks! ulog.Contextual().Info("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 { + if metricsCollector.GetTaskFailed() == metricsCollector.GetTaskTotal() { err := errors.New("all tasks failed") return err }Also applies to: 325-325
🧹 Nitpick comments (3)
Makefile (1)
103-116: Integration test target looks good with a minor suggestion.The implementation correctly checks for the existence of test files before running. Consider adding a non-zero exit if no tests are found when running in CI, to catch missing integration tests intentionally.
🔎 Optional: Exit with error in CI when no integration tests exist
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 ""; \ + if [ -n "$$CI" ]; then exit 1; fi; \ fipkg/config/job.go (1)
20-24: Consider adding validation for WorkerCount.The
WorkerCountfield accepts any integer value including zero or negative numbers, which could cause issues at runtime. Consider adding aValidate()method or bounds checking.🔎 Optional: Add a Validate method
// Validate checks if the JobConfig values are valid. func (c *JobConfig) Validate() error { if c.WorkerCount < 1 { return fmt.Errorf("worker-count must be at least 1, got %d", c.WorkerCount) } return nil }pkg/config/job_test.go (1)
43-64: LGTM on flag parsing tests.The test validates that parsed flag values are correctly bound to the config struct. The TODO for edge case tests (line 63) aligns with the optional validation suggestion for
WorkerCount.Would you like me to generate the edge case tests mentioned in the TODO comment (negative worker count, zero values, etc.)?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.golangci.yml(2 hunks)Makefile(3 hunks)OWNERS(1 hunks)pkg/config/job.go(1 hunks)pkg/config/job_test.go(1 hunks)pkg/job/job.go(11 hunks)pkg/job/metrics.go(3 hunks)pkg/job/trace_context.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pkg/config/job_test.go (1)
pkg/config/job.go (1)
NewJobConfig(13-18)
pkg/job/job.go (2)
pkg/job/metrics.go (3)
MetricsReporter(11-13)NewMetricsCollector(26-28)MetricsCollector(17-23)pkg/job/trace_context.go (1)
AddTraceContext(29-41)
🪛 checkmake (0.2.2)
Makefile
[warning] 94-94: Target body for "test" exceeds allowed length of 5 (6).
(maxbodylength)
🪛 GitHub Actions: CI
Makefile
[error] 1-1: Binary build failed: directory not found for output path '/home/runner/work/adapter-pull-secret/adapter-pull-secret/cmd/pull-secret'. Command: 'CGO_ENABLED=0 go build -o pull-secret ./cmd/pull-secret' (triggered by 'make binary'). Exit code: 2.
🔇 Additional comments (16)
OWNERS (2)
2-2: Verify the numeric user ID works with your OWNERS tooling.The entry
"86254860"appears to be a GitHub user ID rather than a username. Depending on your Prow/OWNERS configuration, numeric IDs may not be resolved correctly for approvals and review assignments. Consider using the actual GitHub username instead.Also applies to: 19-19
1-33: LGTM overall.The OWNERS file structure is correct for Prow-based approval workflows. The approvers and reviewers lists are identical, which is a valid configuration.
Makefile (1)
93-101: LGTM on test target modification.Excluding the
./testdirectory from unit tests is appropriate to separate unit tests from integration tests. Thegrep -v '/test'pattern correctly filters out the test directory..golangci.yml (2)
4-5: LGTM on golangci-lint v2 migration.The configuration correctly uses the v2 format with the new
formatterssection forgofmtandgoimports, which are no longer regular linters in v2.Also applies to: 22-25
29-33: Remove thecheck-shadowingentry from the govet disable list.
check-shadowingis v1 syntax that was replaced withenable: - shadowin golangci-lint v2. The linedisable: - check-shadowingis redundant—onlyenable: - shadowis needed.Likely an incorrect or invalid review comment.
pkg/config/job.go (1)
6-10: LGTM on JobConfig struct.Clean struct definition with appropriate JSON tags for serialization.
pkg/config/job_test.go (2)
9-23: LGTM on default value tests.Good coverage of the constructor defaults and nil check.
25-41: LGTM on flag registration tests.The test correctly verifies that both flags are registered with the FlagSet.
pkg/job/trace_context.go (1)
27-41: LGTM! Thread-safe context enrichment implementation.The lazy callback registration with mutex protection ensures thread-safe operation while avoiding initialization dependencies. The implementation correctly enriches the context with trace information.
pkg/job/metrics.go (3)
35-47: LGTM! Properly synchronized counter increments.Both
IncTaskSuccessandIncTaskFailedcorrectly use mutex locks to ensure thread-safe increments from concurrent workers.
49-61: LGTM! Thread-safe snapshot implementation.The
Snapshotmethod correctly locks the mutex to capture a consistent point-in-time view of all metrics fields.
67-72: LGTM! Snapshot ensures consistent metrics reporting.The reporter correctly obtains a snapshot before logging, which guarantees that all reported values represent the same point in time.
pkg/job/job.go (4)
51-85: LGTM! Clean fluent builder pattern.The setter methods correctly return
*CommandBuilderfor method chaining and provide a clear API for configuring job execution.
106-108: LGTM! Good defensive programming with default reporter.Setting a default
StdoutReporterwhen none is provided ensures metrics are always reported, preventing silent failures.
130-148: LGTM! Straightforward job registry implementation.The registry provides a simple and effective way to manage job registration with proper nil checking.
260-332: LGTM! Well-structured job execution with proper lifecycle management.The
jobRunnercorrectly orchestrates:
- Lifecycle hooks (BeforeJob/AfterJob)
- Panic handling with appropriate re-throw behavior in the main goroutine
- Worker pool coordination with WaitGroup
- Metrics reporting after task completion
| taskCtx := AddTraceContext(ctx, "workerId", strconv.Itoa(workerId)) | ||
| taskCtx = AddTraceContext(taskCtx, "taskName", task.TaskName()) | ||
| taskCtx = AddTraceContext(taskCtx, "taskId", taskId) | ||
| taskCtx = AddTraceContext(taskCtx, "taskId", taskID) |
There was a problem hiding this comment.
Complete the taskId → taskID refactoring.
The context key still uses "taskId" with lowercase 'd', but Go naming conventions prefer "taskID" with uppercase 'ID'. The AI summary indicates this refactoring was intended but appears incomplete at these locations.
🔎 Apply this diff to use consistent naming:
- taskCtx = AddTraceContext(taskCtx, "taskId", taskID)
+ taskCtx = AddTraceContext(taskCtx, "taskID", taskID)- logger.NewOCMLogger(taskCtx).Contextual().Info("Processing task", "workerId", workerId, "taskId", taskID)
+ logger.NewOCMLogger(taskCtx).Contextual().Info("Processing task", "workerId", workerId, "taskID", taskID)Also applies to: 225-225
🤖 Prompt for AI Agents
In pkg/job/job.go around lines 213 and 225 the context key still uses "taskId"
(lowercase 'd') while the refactor intends "taskID" (uppercase 'ID'); change the
string literal keys passed to AddTraceContext from "taskId" to "taskID" at both
locations so the context key naming is consistent with Go conventions and the
earlier refactor.
| pool := workerPool{Queue: taskQueue, Workers: workerCount, PanicHandler: nil, MetricsCollector: metricsCollector} | ||
| pool.Run(ctx) | ||
|
|
||
| if metricsCollector.taskTotal == 0 { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use getter methods for consistent encapsulation.
Same encapsulation issue as in jobRunner: direct field access should be replaced with getter methods to maintain the thread-safety contract of MetricsCollector.
🔎 Update TestRunner to use getter methods:
- if metricsCollector.taskTotal == 0 {
+ if metricsCollector.GetTaskTotal() == 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 {
+ if metricsCollector.GetTaskFailed() == metricsCollector.GetTaskTotal() {
err := errors.New("all tasks failed")
return err
}Also applies to: 362-362
🤖 Prompt for AI Agents
In pkg/job/job.go around lines 357 and 362, the code accesses MetricsCollector
fields directly (e.g., metricsCollector.taskTotal) which violates the
encapsulation/thread-safety contract; replace direct field reads with the
provided getter methods (e.g., metricsCollector.TaskTotal() or the actual getter
name used in the type) so all reads go through the thread-safe accessors,
updating both occurrences at lines 357 and 362 to call the getters.
| // SetTaskTotal sets the total number of tasks. | ||
| func (m *MetricsCollector) SetTaskTotal(total uint32) { | ||
| m.taskTotal = total | ||
| } |
There was a problem hiding this comment.
Add mutex lock to ensure thread-safety consistency.
SetTaskTotal doesn't lock the mutex while modifying taskTotal, but Snapshot() locks when reading it. While the current usage happens to be safe (called before workers start), this creates an inconsistent thread-safety contract for the MetricsCollector API.
🔎 Apply this diff to add proper locking:
func (m *MetricsCollector) SetTaskTotal(total uint32) {
+ m.mu.Lock()
+ defer m.mu.Unlock()
m.taskTotal = total
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SetTaskTotal sets the total number of tasks. | |
| func (m *MetricsCollector) SetTaskTotal(total uint32) { | |
| m.taskTotal = total | |
| } | |
| // SetTaskTotal sets the total number of tasks. | |
| func (m *MetricsCollector) SetTaskTotal(total uint32) { | |
| m.mu.Lock() | |
| defer m.mu.Unlock() | |
| m.taskTotal = total | |
| } |
🤖 Prompt for AI Agents
In pkg/job/metrics.go around lines 30 to 33, SetTaskTotal modifies m.taskTotal
without acquiring the mutex while Snapshot reads it under lock; add proper
locking to make the API thread-safe by acquiring m.mu.Lock() at the start of
SetTaskTotal and deferring m.mu.Unlock() (or unlocking after assignment) so the
write is protected consistently with Snapshot.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.