OSAC-1571: add template_parameters field to BareMetalInstance API - #762
Conversation
|
@mennyaboush: This pull request references OSAC-1571 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 task 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
More reviews will be available in 50 minutes and 24 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughAdds immutable ChangesBareMetalInstance template_parameters end-to-end
Sequence Diagram(s)sequenceDiagram
participant Client
participant PrivateBareMetalInstancesServer
participant templatesDao
participant Reconciler
Client->>PrivateBareMetalInstancesServer: CreateBareMetalInstance(spec.template_parameters)
PrivateBareMetalInstancesServer->>templatesDao: Get(templateId)
templatesDao-->>PrivateBareMetalInstancesServer: BareMetalInstanceTemplate
PrivateBareMetalInstancesServer->>PrivateBareMetalInstancesServer: validateAndApplyTemplateParameters()
PrivateBareMetalInstancesServer-->>Client: created instance
Client->>PrivateBareMetalInstancesServer: UpdateBareMetalInstance(mask includes template_parameters)
PrivateBareMetalInstancesServer->>PrivateBareMetalInstancesServer: validateImmutability()
PrivateBareMetalInstancesServer-->>Client: InvalidArgument if changed
Reconciler->>Reconciler: mutateBMI()
Reconciler->>Reconciler: ConvertTemplateParametersToJSON()
Reconciler->>Reconciler: merge sshPublicKey and userDataSecret
Reconciler-->>Reconciler: marshal hub CR TemplateParameters
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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: 4
🤖 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/controllers/baremetalinstance/baremetalinstance_reconciler_function.go`:
- Around line 523-525: The parameter name used in the BareMetalInstance
reconciler is outdated: update the ssh key assignment in the BareMetalInstance
reconciliation path to use the renamed system parameter key expected by the
template. In the code that builds params from
t.bareMetalInstance.GetSpec().HasSshPublicKey(), replace the old sshKey entry
with sshPublicKey so the server-managed key is propagated correctly and can
override any user-supplied template_parameters value under that name.
In `@internal/servers/private_baremetal_instances_server.go`:
- Around line 262-269: The validateAndApplyTemplateParameters method currently
returns early when templateID is empty, which allows spec.template_parameters to
slip through unvalidated. Update this path to reject any non-empty
bmi.GetSpec().GetTemplateParameters() when no template is available, and keep
the existing parameter validation/defaulting flow only for cases where
templateID is set. Use validateAndApplyTemplateParameters and providedParams as
the key locations for the fix.
In `@internal/utils/template_parameters_test.go`:
- Around line 241-261: Add test coverage for the bare metal default-application
path in the template parameter tests: the current
BareMetalInstanceTemplateAdapter validation only checks
ValidateTemplateParameters, so it can miss regressions in
ProcessTemplateParametersWithDefaults. Extend the existing bare metal test suite
to cover ProcessTemplateParametersWithDefaults using BareMetalInstanceTemplate
and BareMetalInstanceTemplateParameterDefinition.Default, asserting that a
missing parameter is populated from the default before validation continues.
In `@proto/public/osac/public/v1/baremetal_instance_type.proto`:
- Around line 62-63: The ProtoJSON documentation link in the comment is written
with reversed Markdown syntax, so it will not render correctly in generated
docs. Update the comment near baremetal_instance_type.proto to use standard link
text formatting, and apply the same fix to the copied comment blocks in the
matching private/template proto definitions so the documentation stays
consistent across all occurrences.
🪄 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: Enterprise
Run ID: dd9dafae-1c77-43ad-ad27-5d0e8a0de28c
⛔ Files ignored due to path filters (8)
internal/api/osac/private/v1/baremetal_instance_template_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/baremetal_instance_template_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/baremetal_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/baremetal_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/baremetal_instance_template_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/baremetal_instance_template_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/baremetal_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/baremetal_instance_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
internal/controllers/baremetalinstance/baremetalinstance_reconciler_function.gointernal/controllers/baremetalinstance/baremetalinstance_reconciler_function_test.gointernal/servers/private_baremetal_instances_server.gointernal/servers/private_baremetal_instances_server_test.gointernal/utils/template_parameters.gointernal/utils/template_parameters_test.goproto/private/osac/private/v1/baremetal_instance_template_type.protoproto/private/osac/private/v1/baremetal_instance_type.protoproto/public/osac/public/v1/baremetal_instance_template_type.protoproto/public/osac/public/v1/baremetal_instance_type.proto
c33c052 to
a91f282
Compare
a91f282 to
b08d861
Compare
Signed-off-by: Menny Aboush <maboush@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com>
b08d861 to
7ce2666
Compare
Address PR review feedback from adriengentil: add tests covering CatalogItem field_definitions alongside template_parameters. Signed-off-by: Menny Aboush <maboush@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
Signed-off-by: Menny Aboush <maboush@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: MENNY ABOUSH <maboush@maboush-thinkpadt14gen5.raanaii.csb>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, mennyaboush 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 |
3 participants
Summary
template_parameters(map<string, google.protobuf.Any>) toBareMetalInstanceSpecin both public and private APIs, allowing tenants to pass custom parameters to the provisioning template at instance creation timeBareMetalInstanceTemplateParameterDefinitionandparametersfield toBareMetalInstanceTemplatefor server-side parameter validationtemplateParametersJSON alongside system parameters (sshPublicKey, userDataSecret), with system params overriding user values for securityDependencies
sshKey→sshPublicKeyin reconciler). Reconciler tests assertsshPublicKeykey name.Companion PR
Test plan
buf lintpassesgo build ./...passesgofmt -s -w .— no formatting issuesginkgo run internal/utils— 57 tests pass (1 new adapter test)ginkgo run internal/servers— 1042 tests pass (7 new template_parameters tests)ginkgo run internal/controllers/baremetalinstance— 40 tests pass (3 new reconciler tests)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Anyvalues, and default values derived from templates.Bug Fixes
Tests