fix(EC-1994): correct off-by-one in validate image worker loop - #3425
fix(EC-1994): correct off-by-one in validate image worker loop#3425cuipinghuo wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe image validator now starts exactly the configured number of workers. The Konflux and enterprise contract pipeline defaults and documentation now use updated ChangesWorker count alignment
Estimated code review effort: 1 (Trivial) | ~2 minutes Mergeability Score: ⚪ Minimal · up to The worker loop now creates the configured number of workers, with defaults and documentation updated consistently; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
🤖 Review · |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml`:
- Line 180: Synchronize the README worker defaults with the Task definitions:
for tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml lines
180-180, document the effective default as 5 instead of 35/unused; for
tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml lines
186-186, update the README default from 1 to 2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8ffe48a4-a59b-47ad-aa6c-96664ec08be3
📒 Files selected for processing (2)
tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
|
🤖 Finished Review · ✅ Success · Started 2:16 PM UTC · Completed 2:31 PM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsLow
Labels: Bug fix in CLI validate command with documentation updates |
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
ready-for-merge
|
The change looks good, I'm just unsure of the impact to the users that modify the |
|
🤖 Finished Review · ❌ Failure · Started 8:26 PM UTC · Completed 8:40 PM UTC |
An off-by-one fix in conforma/cli (conforma/cli#3425) corrected the worker loop from <= to <, reducing actual worker count by 1. Bump the fallback default to preserve the effective concurrency users had before the fix. This only affects releases where conforma.workerCount is not set in konflux-release-data. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: cuipinghuo <chuo@redhat.com>
The worker loop used <= instead of <, spawning numWorkers+1 goroutines instead of the intended numWorkers. Bump task defaults to preserve the effective concurrency users had before the fix: - verify-conforma-konflux-ta: 4 → 5 - verify-enterprise-contract: 1 → 2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
🤖 Finished Review · ✅ Success · Started 6:54 PM UTC · Completed 7:08 PM UTC Commit: |
| // Initialize each worker. They will wait patiently until a job is sent to the jobs | ||
| // channel, or the jobs channel is closed. | ||
| for i := 0; i <= numWorkers; i++ { | ||
| for i := 0; i < numWorkers; i++ { |
There was a problem hiding this comment.
[low] test-adequacy
No test covers the worker loop to verify that exactly numWorkers goroutines are spawned. The off-by-one existed since the loop was introduced and was not caught by tests. A regression test would prevent reintroduction.
Suggested fix: Consider adding a test that asserts the number of concurrent workers matches the --workers flag value.
There was a problem hiding this comment.
I'm not sure that's worth it.
fullsend-ai-review
Bot
removed
the
ready-for-merge
fullsend-ai-review
Bot
added
the
requires-manual-review
|
Rebase should fix the EC violation now that #3496 is merged. |
An off-by-one fix in conforma/cli (conforma/cli#3425) corrected the worker loop from <= to <, reducing actual worker count by 1. Bump the default to preserve the effective concurrency users had before the fix. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 participants
Summary
cmd/validate/image.go:404:<=changed to<so the loop spawns exactlynumWorkersgoroutines instead ofnumWorkers+1<=loop spawned N+1 workers for a configured value of N):verify-conforma-konflux-ta: 4 → 5verify-enterprise-contract: 1 → 2Test plan
go build ./cmd/validate/passesgo test -tags=unit ./cmd/validate/ -run TestValidateImagepassesmake generateproduces no additional diffResolves: https://redhat.atlassian.net/browse/EC-1994
🤖 Generated with Claude Code