OSAC-2493: add --set CLI flag and enforce field_definitions on template_parameters - #953
Conversation
|
@alosadagrande: This pull request references OSAC-2493 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughThe change adds repeatable ChangesCatalog template parameter support
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant CatalogItem
participant ApplyFields
participant CatalogValidation
participant ResourceAPI
CLI->>CatalogItem: request creation with --catalog-item
CatalogItem-->>CLI: generated specification
CLI->>ApplyFields: apply repeatable --set overrides
ApplyFields-->>CLI: updated specification
CLI->>CatalogValidation: submit catalog-derived values
CatalogValidation-->>ResourceAPI: validated resource request
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/servers/catalog_item_validation.go (1)
90-97: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftEnforce per-parameter field definitions. Empty definitions and a parent
template_parametersdefinition can still authorize arbitrary parameter keys.
internal/servers/catalog_item_validation.go#L90-L97: reject template parameters unless each key has an exacttemplate_parameters.<name>definition; do not let the parent map cover descendants.internal/servers/catalog_item_validation_test.go#L411-L447: add empty-definition and parent-definition rejection coverage.🤖 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/servers/catalog_item_validation.go` around lines 90 - 97, Update the allowed-path construction in catalog item validation so template parameter keys are authorized only by exact template_parameters.<name> field definitions, never by an empty definition or the parent template_parameters definition; add rejection coverage for both cases in internal/servers/catalog_item_validation_test.go:411-447.
🤖 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 `@docs/CATALOG_ITEMS.md`:
- Around line 317-324: Update the --set documentation to remove the
Helm-compatible parsing claim and describe the actual supported format: each
occurrence accepts one KEY=VALUE pair, split at the first equals sign. Keep the
existing template parameter examples and command usage unchanged.
In `@internal/cmd/cli/create/fieldutil/fieldutil.go`:
- Around line 95-97: Preserve exact CLI integer overrides in the field parsing
logic by returning the parsed integer or a string-backed json.Number instead of
converting it through strconv.ParseFloat. Update the affected expectation in
internal/cmd/cli/create/fieldutil/fieldutil_test.go:53-57 and add a regression
covering 9007199254740993; the production change belongs in
internal/cmd/cli/create/fieldutil/fieldutil.go:95-97.
---
Outside diff comments:
In `@internal/servers/catalog_item_validation.go`:
- Around line 90-97: Update the allowed-path construction in catalog item
validation so template parameter keys are authorized only by exact
template_parameters.<name> field definitions, never by an empty definition or
the parent template_parameters definition; add rejection coverage for both cases
in internal/servers/catalog_item_validation_test.go:411-447.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 45092268-9433-4ac4-bc08-2328e92e3f70
📒 Files selected for processing (10)
docs/CATALOG_ITEMS.mdinternal/cmd/cli/create/baremetalinstance/create_bare_metal_instance_cmd.gointernal/cmd/cli/create/cluster/create_cluster_cmd.gointernal/cmd/cli/create/computeinstance/create_compute_instance_cmd.gointernal/cmd/cli/create/fieldutil/fieldutil.gointernal/cmd/cli/create/fieldutil/fieldutil_suite_test.gointernal/cmd/cli/create/fieldutil/fieldutil_test.gointernal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/private_baremetal_instances_server_test.go
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmd/cli/create/fieldutil/fieldutil.go (1)
67-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve the original spec when applying fields fails.
proto.Reset(spec)runs beforeprotojson.Unmarshal(updatedJSON, spec), so an invalid JSON generated by--setcan return an error after clearing the caller’s message. Unmarshal into a reset temporary message first, then replacespeconly on success.🤖 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/cmd/cli/create/fieldutil/fieldutil.go` around lines 67 - 69, Update the field-application flow around protojson.Unmarshal to unmarshal updatedJSON into a reset temporary message first, preserving the original spec when unmarshalling fails. Replace the caller’s spec only after successful unmarshalling, while retaining the existing error wrapping.
🤖 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.
Outside diff comments:
In `@internal/cmd/cli/create/fieldutil/fieldutil.go`:
- Around line 67-69: Update the field-application flow around
protojson.Unmarshal to unmarshal updatedJSON into a reset temporary message
first, preserving the original spec when unmarshalling fails. Replace the
caller’s spec only after successful unmarshalling, while retaining the existing
error wrapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b0fd62ac-2198-4d39-8268-fa9186e050ca
📒 Files selected for processing (3)
docs/CATALOG_ITEMS.mdinternal/cmd/cli/create/fieldutil/fieldutil.gointernal/cmd/cli/create/fieldutil/fieldutil_test.go
28e9937 to
bcd4cc8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@internal/cmd/cli/create/fieldutil/fieldutil.go`:
- Around line 98-100: The float inference logic in inferValue and inferAnyType
must recognize exponent-form values such as 1e3, 1E3, and -2E-4 as doubles;
apply the decimal-point count restriction only to non-exponent forms. Update
fieldutil.go at lines 98-100 and 111-113, and add exponent-form coverage to
fieldutil_test.go at line 57 for both inferValue and inferAnyType.
In `@internal/servers/catalog_item_validation.go`:
- Around line 91-92: The applyFieldDefinitions logic in
internal/servers/catalog_item_validation.go at lines 91-92 must enforce the
template-parameter allowlist even when field_definitions is empty, while
retaining only catalog_item and template as implicitly allowed; update the
early-return flow accordingly. Add coverage in
internal/servers/catalog_item_validation_test.go at lines 411-447 verifying that
an empty field_definitions list rejects template_parameters.vpc_id.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 284e0905-ca44-4690-8f2d-237755c72f00
📒 Files selected for processing (10)
docs/CATALOG_ITEMS.mdinternal/cmd/cli/create/baremetalinstance/create_bare_metal_instance_cmd.gointernal/cmd/cli/create/cluster/create_cluster_cmd.gointernal/cmd/cli/create/computeinstance/create_compute_instance_cmd.gointernal/cmd/cli/create/fieldutil/fieldutil.gointernal/cmd/cli/create/fieldutil/fieldutil_suite_test.gointernal/cmd/cli/create/fieldutil/fieldutil_test.gointernal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/private_baremetal_instances_server_test.go
|
/retest |
|
Re-triggered failed runs:
|
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Overall the PR is well-structured - the CLI --set flag design, protobuf Any wrapping, and server-side validation tightening all look solid. One UX issue with error messages when template parameters are rejected.
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 1 |
| 💡 Suggestion | 0 |
Remove template_parameters from the allowedPaths bypass in applyFieldDefinitions. Template parameters must now be listed in field_definitions to be accepted, giving admins control over which Ansible extra variables tenants can set. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
Add a --set KEY=VALUE flag to cluster, compute-instance, and bare-metal-instance create commands. The flag allows users to set spec fields and template parameters when creating resources from catalog items. Parsing follows the same approach as helm --set: split on the first '=' only, preserving values that contain '='. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
Signed-off-by: Alberto Losada Grande <alosadag@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
Simplify inferValue logic in fieldutil for readability without changing behavior. Fix typo in CATALOG_ITEMS.md. Update 4 bare metal tests to include template_parameters in field_definitions, adapting them to the new validation behavior. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
Return int64 directly from inferValue instead of converting to float64, preserving precision for large integers. Remove misleading Helm parsing claim from docs. Add regression test for int64 precision. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
…aths Stop recursing into protobuf Any wrapper keys (@type, value) when collecting leaf paths for field_definitions validation. This produces user-friendly error messages like "template_parameters.vpc_id" instead of leaking internal encoding details. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
92dbebe to
7b6ce85
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/servers/catalog_item_validation.go (1)
90-97: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject the parent template-parameter path.
validateFieldDefinitionscurrently allowstemplate_parametersalone, andisPathCoveredthen permits everytemplate_parameters.*value. Reject the baretemplate_parameterspath unless the allowed template parameters are explicitly enumerated below it.🤖 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/servers/catalog_item_validation.go` around lines 90 - 97, Update validateFieldDefinitions and its allowedPaths setup to exclude the bare template_parameters path, while still permitting explicitly enumerated template_parameters.* paths. Ensure isPathCovered cannot treat template_parameters alone as covering every descendant value.Source: Path instructions
🤖 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 `@internal/servers/catalog_item_validation.go`:
- Line 67: Qualify the comment near applyFieldDefinitions in
internal/servers/catalog_item_validation.go to state that unknown spec fields
are rejected only when field_definitions is non-empty. Update
docs/CATALOG_ITEMS.md sections covering omitted template parameters and required
parameters to document that an absent or empty field_definitions list preserves
unrestricted template-parameter access subject to template-level validation, so
required parameters need listing only when definitions are provided.
- Around line 201-224: Update wrapValueAsAny and its callers to derive the
generated protobuf wrapper type from each template parameter’s declared type
instead of inferring all numeric values as Int64Value or DoubleValue. Preserve
the corresponding value representation for Int32Value, FloatValue, UInt64Value,
and other supported wrappers, and add coverage in
internal/servers/catalog_item_validation_test.go:205-225 for these non-Int64
numeric types; apply the root fix in
internal/servers/catalog_item_validation.go:201-224.
---
Outside diff comments:
In `@internal/servers/catalog_item_validation.go`:
- Around line 90-97: Update validateFieldDefinitions and its allowedPaths setup
to exclude the bare template_parameters path, while still permitting explicitly
enumerated template_parameters.* paths. Ensure isPathCovered cannot treat
template_parameters alone as covering every descendant value.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e8c648c-0a47-473b-a2fe-4d629c9859c6
📒 Files selected for processing (10)
docs/CATALOG_ITEMS.mdinternal/cmd/cli/create/baremetalinstance/create_bare_metal_instance_cmd.gointernal/cmd/cli/create/cluster/create_cluster_cmd.gointernal/cmd/cli/create/computeinstance/create_compute_instance_cmd.gointernal/cmd/cli/create/fieldutil/fieldutil.gointernal/cmd/cli/create/fieldutil/fieldutil_suite_test.gointernal/cmd/cli/create/fieldutil/fieldutil_test.gointernal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/private_baremetal_instances_server_test.go
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Re-review
Previous finding (protobuf Any internals in error message): FIXED - collectLeafPaths now treats template_parameters entries as opaque leaves.
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 1 |
| 💡 Suggestion | 1 |
💡 wrapValueAsAny and inferAnyType duplicate type-mapping logic - Both map values to protobuf Any @type URLs with hardcoded strings in catalog_item_validation.go and fieldutil.go respectively. Different input types (any vs string) make full consolidation non-trivial, but shared constants for the type URLs would reduce coupling.
Move duplicated map traversal helpers into a shared package to avoid divergence between the CLI (fieldutil) and server (catalog_item_validation) copies. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alosadagrande, tzvatot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This branch was previously deployed
3 participants
Summary
field_definitionsontemplate_parameters:template_parametersis no longer implicitly allowed. When a resource is created from a catalog item, everytemplate_parameters.<name>path must be listed infield_definitions— just like typed spec fields. This gives admins explicit control over which template parameters users can set.--set KEY=VALUEflag toosac create cluster,osac create computeinstance, andosac create baremetalinstance. Works likehelm --set: type inference (bool, int, float, string), protobuf Any wrapping fortemplate_parameters.*paths, nested dot-notation for spec fields. Only supported with--catalog-item.wrapValueAsAny/unwrapAnyValuefor converting between Go values and protobuf Any JSON format when applying defaults fromfield_definitions.Breaking change
Jira
OSAC-2493
Test plan
gofmtcleango build ./...)catalog_item_validation_test.gotests covering field_definitions + template_parameters scenariosfieldutil_test.gotests covering--setparsing, type inference, and proto round-trip🤖 Generated with Claude Code
Summary by CodeRabbit
--set KEY=VALUEoverrides for catalog-item-based cluster and compute instance creation (dot notation, includingtemplate_parameters.*);--setis only allowed with--catalog-item.--setsupport for bare metal instance spec fields and template parameters.field_definitions/template_parametersbehavior, refreshed available-path tables, and updated CLI examples/outcomes.template_parameters.*, including typed handling and correct rejection/acceptance for editable vs non-editable parameters.--setapplication and template-parameter validation.