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 (7)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe image validator now starts exactly the configured number of workers. The Konflux and enterprise contract Tekton Tasks update their default ChangesWorker parallelism
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 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.
🚀 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 runReviewFindingsLow
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 |
| * **HOMEDIR**: Value for the HOME environment variable. (default: "/tekton/home") | ||
| * **EFFECTIVE_TIME**: Run policy checks with the provided time. (default: "now") | ||
| * **EXTRA_RULE_DATA**: Merge additional Rego variables into the policy data. Use syntax "key=value,key2=value2..." (default: "") | ||
| * **WORKERS**: Number of parallel workers to use for policy evaluation. This parameter is currently not used. All policy evaluations are run with 35 workers. (default: "35") |
There was a problem hiding this comment.
This was some left-over incorrect documentation, right??
There was a problem hiding this comment.
Yes, it was stale/incorrect documentation. It was introduced in 5302c52 (fix(ec-1404)) — that commit rewrote this README line to say "This parameter is currently not used. All policy evaluations are run with 35 workers." The "35" likely came from conforma.workerCount: 35 which is set in ~107 ReleasePlanAdmission CRDs in konflux-release-data. However, that workerCount field is never wired to the task's WORKERS param — the release pipelines in release-service-catalog don't pass a WORKERS param at all, so the task YAML default always controlled --workers. The README was wrong from the moment it was written.
There was a problem hiding this comment.
Correction to my previous comment: my local clone of release-service-catalog was stale. The upstream konflux-ci/release-service-catalog does pass WORKERS in the managed pipelines (under pipelines/managed/), e.g. https://github.com/konflux-ci/release-service-catalog/blob/main/pipelines/managed/rh-advisories/rh-advisories.yaml — the value comes from $(tasks.collect-task-params.results.extractedValues[0]), which extracts conforma.workerCount: 35 from konflux-release-data.
So for release pipelines, the task default doesn't matter — it gets overridden to 35 (which with the old <= bug meant 36 actual goroutines). The task default only applies when the task is invoked without an explicit WORKERS param (e.g. integration pipelines via verify-conforma-konflux-ta).
The README text "This parameter is currently not used" was still misleading though — the parameter is used, it's just that the release pipelines override it.
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 · ❌ Failure · Started 8:26 PM UTC · Completed 8:40 PM UTC |
4 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