feat: add per-target validation hints to aib-target-defaults ConfigMap#302
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR extends the operator's target default configuration system with validation hints. ChangesTarget defaults validation hints
Sequence DiagramsequenceDiagram
participant Server as Build API Server
participant Validation as validateTargetDefaults
participant CLI as CLI (caib build)
participant Warning as warnIfNotInList
Server->>Server: Parse target-defaults.yaml
Server->>Validation: Validate loaded TargetDefaults
Validation->>Validation: Check Architecture in AcceptedArchitectures
Validation->>Validation: Check DefaultFormat in AcceptedFormats
Validation-->>Server: Return validation result
CLI->>CLI: Fetch operator config via /v1/config
CLI->>CLI: Apply TargetDefaults to request
CLI->>Warning: Check selected Architecture
Warning-->>CLI: Warn if not in AcceptedArchitectures
CLI->>Warning: Check selected ExportFormat
Warning-->>CLI: Warn if not in AcceptedFormats
CLI->>CLI: Proceed with build
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/buildapi/build_validation_test.go (1)
198-293: 💤 Low valueNote: Tests and implementation added together.
As per coding guidelines, tests should be added before starting implementation. Both the
validateTargetDefaultsimplementation and its tests appear together in this PR. While the test coverage is excellent, consider following a test-first approach in future changes.🤖 Prompt for 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. In `@internal/buildapi/build_validation_test.go` around lines 198 - 293, The PR added both the validateTargetDefaults implementation and its tests together; per our guideline, split them so tests land before implementation. Create one change that adds the test cases (all Describe/It blocks referencing validateTargetDefaults) without adding or modifying the implementation, then follow with a second change that implements validateTargetDefaults (or modifies it) to satisfy those tests; ensure the tests reference the same function name validateTargetDefaults so CI runs the failing tests first and reviewers can verify behavior before implementation.
🤖 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.
Nitpick comments:
In `@internal/buildapi/build_validation_test.go`:
- Around line 198-293: The PR added both the validateTargetDefaults
implementation and its tests together; per our guideline, split them so tests
land before implementation. Create one change that adds the test cases (all
Describe/It blocks referencing validateTargetDefaults) without adding or
modifying the implementation, then follow with a second change that implements
validateTargetDefaults (or modifies it) to satisfy those tests; ensure the tests
reference the same function name validateTargetDefaults so CI runs the failing
tests first and reviewers can verify behavior before implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 34fdc2cd-ab78-4102-8605-a44557d97e19
📒 Files selected for processing (7)
cmd/caib/buildcmd/build.gointernal/buildapi/build_validation.gointernal/buildapi/build_validation_test.gointernal/buildapi/server.gointernal/buildapi/server_test.gointernal/buildapi/types.gointernal/controller/operatorconfig/controller.go
maboras-rh
left a comment
maboras-rh
left a comment
There was a problem hiding this comment.
lgtm. other than the two small comments
| checkInList(&errs, name, "defaultFormat", td.DefaultFormat, "acceptedFormats", td.AcceptedFormats) | ||
| } | ||
| if len(errs) > 0 { | ||
| return fmt.Errorf("%s", strings.Join(errs, "; ")) |
There was a problem hiding this comment.
| return fmt.Errorf("%s", strings.Join(errs, "; ")) | |
| return errors.New(strings.Join(errs, "; ")) |
| return | ||
| } | ||
| if !slices.Contains(accepted, value) { | ||
| fmt.Fprintf(os.Stderr, "Warning: %s %q is not in accepted values %v\n", field, value, accepted) |
There was a problem hiding this comment.
warnIfNotInList writes directly to os.Stderr and the rest of build.go uses clilog.Infof(...) for output. i think that warnings should go through the same logging abstraction for consistency. If clilog doesn't have a Warnf, add one or use clilog.Infof("Warning: ...")
WDYT?
bennyz
force-pushed
the
aib-validation
branch
2 times, most recently
from
1deeaef to
28bd015
Compare
Each target now declares acceptedFormats and acceptedArchitectures, enabling server-side validation of ConfigMap entries at parse time and CLI warnings for user-supplied values before build submission. Assisted-by: claude-opus-4.6 Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
2 participants
Each target now declares acceptedFormats and acceptedArchitectures, enabling server-side validation of ConfigMap entries at parse time and CLI warnings for user-supplied values before build submission.
Summary
Related Issues
Type of Change
Testing
make test)make lint)make manifests generate)Summary by CodeRabbit
Release Notes
New Features
Tests