OSAC-1531: add catalog item examples and documentation - #972
openshift-merge-bot[bot] merged 5 commits into
Conversation
|
@danielerez: This pull request references OSAC-1531 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 feature 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. |
03d2574 to
e119138
Compare
WalkthroughAdds documentation for creating example OSAC resources and introduces cluster and VM catalog item YAML examples with template references, publication settings, configurable fields, defaults, and validation schemas. ChangesCatalog examples
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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 |
e119138 to
c66a2fc
Compare
c66a2fc to
eda17c6
Compare
![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 `@examples/catalog-items/linux-vm.yaml`:
- Around line 22-26: Replace the duplicated image.source_ref validation_schema
in examples/catalog-items/linux-vm.yaml (lines 22-26),
examples/catalog-items/linux-vm-gpu.yaml (lines 22-26),
examples/catalog-items/windows-11-vm.yaml (lines 27-31), and
examples/catalog-items/windows-server-vm.yaml (lines 27-31) with the project’s
real container-image reference validator, preserving the existing field
configuration while accepting registry ports, uppercase tags, and digest
selectors.
In `@examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml`:
- Around line 12-19: The CIDR validation schema only checks formatting, allowing
semantically invalid network addresses. Update the validation_schema for both
spec.network.pod_cidr and spec.network.service_cidr in
examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml lines 12-19,
examples/catalog-items/ocp-4-20-nico-baremetal-cluster.yaml lines 13-20,
examples/catalog-items/ocp-ci-cluster.yaml lines 12-19, and
examples/catalog-items/simple-ocp-4-17-cluster.yaml lines 13-20 to use semantic
CIDR validation that enforces valid IPv4 octets and prefix lengths.
In `@examples/README.md`:
- Around line 82-92: Update the “File format” field list in the README to
include the required `default` property for each entry in `field_definitions`,
keeping the description aligned with the catalog-item contract documented in
CATALOG_ITEMS.md.
- Around line 15-19: Update the token-generation section in the examples
documentation to show how the generated service-account token is supplied to
osac, using the osac login --private --token flow with the target ADDRESS or
linking to the established --token-script flow if reusable authentication is
intended.
🪄 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: f44cd7ad-16bc-4de9-a208-94866d60ac1e
📒 Files selected for processing (9)
examples/README.mdexamples/catalog-items/linux-vm-gpu.yamlexamples/catalog-items/linux-vm.yamlexamples/catalog-items/ocp-4-20-ai-maas-cluster.yamlexamples/catalog-items/ocp-4-20-nico-baremetal-cluster.yamlexamples/catalog-items/ocp-ci-cluster.yamlexamples/catalog-items/simple-ocp-4-17-cluster.yamlexamples/catalog-items/windows-11-vm.yamlexamples/catalog-items/windows-server-vm.yaml
eda17c6 to
304bad3
Compare
Add example YAML files for catalog items with a README explaining how to use `osac create -f` to load them via the private API. Cluster catalog items: - Single Node OpenShift (SNO) - Compact 3-node OpenShift - Simple OpenShift 4.17 (fc430) - OpenShift 4.20 NICo bare metal Compute instance (VM) catalog items: - Linux VM (general-purpose) - Linux VM with GPU - Windows Server VM - Windows 11 VM Signed-off-by: Daniel Erez <derez@redhat.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Daniel Erez <danielerez@gmail.com>
304bad3 to
5ca2a48
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 `@examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml`:
- Around line 12-19: Add the required default property to each affected field
definition: valid CIDR defaults for pod_cidr and service_cidr in
examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml lines 12-19,
ocp-4-20-nico-baremetal-cluster.yaml lines 13-20,
ocp-4-20-openshift-ai-cluster.yaml lines 13-20, ocp-ci-cluster.yaml lines 12-19,
and simple-ocp-4-17-cluster.yaml lines 13-20; add explicit empty defaults for
optional user_data in linux-vm.yaml lines 37-40, linux-vm-gpu.yaml lines 37-40,
windows-11-vm.yaml lines 42-45, and windows-server-vm.yaml lines 42-45.
In `@examples/README.md`:
- Around line 32-33: Update the bulk catalog creation shell loop to fail
immediately when any osac create command returns a nonzero status, using set -e
or explicit failure handling so partial imports cannot be reported as
successful.
🪄 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: ed3bc489-d29b-4796-8eb0-797a89900e00
📒 Files selected for processing (10)
examples/README.mdexamples/catalog-items/linux-vm-gpu.yamlexamples/catalog-items/linux-vm.yamlexamples/catalog-items/ocp-4-20-ai-maas-cluster.yamlexamples/catalog-items/ocp-4-20-nico-baremetal-cluster.yamlexamples/catalog-items/ocp-4-20-openshift-ai-cluster.yamlexamples/catalog-items/ocp-ci-cluster.yamlexamples/catalog-items/simple-ocp-4-17-cluster.yamlexamples/catalog-items/windows-11-vm.yamlexamples/catalog-items/windows-server-vm.yaml
| - path: spec.network.pod_cidr | ||
| display_name: Pod CIDR | ||
| editable: true | ||
| validation_schema: '{"type":"string","pattern":"^([0-9]{1,3}\\.){3}[0-9]{1,3}/[0-9]{1,2}$"}' | ||
| - path: spec.network.service_cidr | ||
| display_name: Service CIDR | ||
| editable: true | ||
| validation_schema: '{"type":"string","pattern":"^([0-9]{1,3}\\.){3}[0-9]{1,3}/[0-9]{1,2}$"}' |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add the required default property to every field definition.
The README and upstream catalog-item contract describe each entry as containing default, but all cluster CIDR fields and VM user_data fields omit it. Add valid CIDR defaults and explicit empty defaults for optional user_data; otherwise osac create -f may reject or create incomplete catalog definitions.
examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml#L12-L19: add defaults to both CIDR fields.examples/catalog-items/ocp-4-20-nico-baremetal-cluster.yaml#L13-L20: add defaults to both CIDR fields.examples/catalog-items/ocp-4-20-openshift-ai-cluster.yaml#L13-L20: add defaults to both CIDR fields.examples/catalog-items/ocp-ci-cluster.yaml#L12-L19: add defaults to both CIDR fields.examples/catalog-items/simple-ocp-4-17-cluster.yaml#L13-L20: add defaults to both CIDR fields.examples/catalog-items/linux-vm.yaml#L37-L40: add auser_datadefault.examples/catalog-items/linux-vm-gpu.yaml#L37-L40: add auser_datadefault.examples/catalog-items/windows-11-vm.yaml#L42-L45: add auser_datadefault.examples/catalog-items/windows-server-vm.yaml#L42-L45: add auser_datadefault.
📍 Affects 9 files
examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml#L12-L19(this comment)examples/catalog-items/ocp-4-20-nico-baremetal-cluster.yaml#L13-L20examples/catalog-items/ocp-4-20-openshift-ai-cluster.yaml#L13-L20examples/catalog-items/ocp-ci-cluster.yaml#L12-L19examples/catalog-items/simple-ocp-4-17-cluster.yaml#L13-L20examples/catalog-items/linux-vm.yaml#L37-L40examples/catalog-items/linux-vm-gpu.yaml#L37-L40examples/catalog-items/windows-11-vm.yaml#L42-L45examples/catalog-items/windows-server-vm.yaml#L42-L45
🤖 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 `@examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml` around lines 12 - 19,
Add the required default property to each affected field definition: valid CIDR
defaults for pod_cidr and service_cidr in
examples/catalog-items/ocp-4-20-ai-maas-cluster.yaml lines 12-19,
ocp-4-20-nico-baremetal-cluster.yaml lines 13-20,
ocp-4-20-openshift-ai-cluster.yaml lines 13-20, ocp-ci-cluster.yaml lines 12-19,
and simple-ocp-4-17-cluster.yaml lines 13-20; add explicit empty defaults for
optional user_data in linux-vm.yaml lines 37-40, linux-vm-gpu.yaml lines 37-40,
windows-11-vm.yaml lines 42-45, and windows-server-vm.yaml lines 42-45.
| # Create all catalog items at once | ||
| for f in examples/catalog-items/*.yaml; do osac create -f "$f"; done |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Fail fast during bulk creation.
If an intermediate osac create fails, the loop continues and the final command can return success, masking a partial import. Add set -e or explicitly exit on failure.
🤖 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 `@examples/README.md` around lines 32 - 33, Update the bulk catalog creation
shell loop to fail immediately when any osac create command returns a nonzero
status, using set -e or explicit failure handling so partial imports cannot be
reported as successful.
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Review
Useful PR adding example YAML files for catalog items with a clear README. However, several examples use incorrect spec. prefixes on field_definition paths, which would cause them to silently fail at resource creation time.
| Category | Count |
|---|---|
| 🔴 Critical | 1 |
| 🟡 Important | 0 |
| 💡 Suggestion | 1 |
💡 PR description lists files not in the diff
The PR body mentions "Single Node OpenShift (SNO)" and "Compact 3-node OpenShift" catalog items, but neither file exists in the diff. Stale description from an earlier revision?
| - path: spec.network.pod_cidr | ||
| display_name: Pod CIDR | ||
| editable: true | ||
| validation_schema: '{"type":"string","pattern":"^([0-9]{1,3}\\.){3}[0-9]{1,3}/[0-9]{1,2}$"}' | ||
| - path: spec.network.service_cidr | ||
| display_name: Service CIDR | ||
| editable: true | ||
| validation_schema: '{"type":"string","pattern":"^([0-9]{1,3}\\.){3}[0-9]{1,3}/[0-9]{1,2}$"}' |
There was a problem hiding this comment.
🔴 spec. prefix on field_definition paths will silently break resource creation
applyFieldDefinitions is called as applyFieldDefinitions(cluster.GetSpec(), ...) - it receives the spec proto, not the full object. When marshaled to JSON, the top-level keys are the spec fields directly (network, node_sets, etc.), NOT wrapped in a spec key.
Paths with spec. prefix resolve to nothing. setNestedValue creates a phantom spec key in the JSON map, which is silently ignored when unmarshaling back to proto. Confirmed by catalog_item_validation_test.go which uses paths WITHOUT spec. prefix (e.g., pull_secret, is_windows, network).
Fix: spec.network.pod_cidr -> network.pod_cidr, spec.network.service_cidr -> network.service_cidr
Same issue in all cluster catalogs (ocp-4-20-nico-baremetal-cluster.yaml, ocp-4-20-openshift-ai-cluster.yaml, ocp-ci-cluster.yaml, simple-ocp-4-17-cluster.yaml) and in Windows VM catalogs where spec.is_windows should be is_windows.
Impact:
- Cluster catalogs: user-provided values at the correct path (
network.pod_cidr) would be rejected as "unlisted in field_definitions" - Windows VMs: non-editable
is_windowsdefaulttruesilently fails to apply, creating Linux-configured VMs instead
There was a problem hiding this comment.
Good catch:) Fixed.
51714cf to
40f226a
Compare
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 `@examples/README.md`:
- Line 48: Update the documented osac create computeinstance command to replace
the angle-bracket subnet placeholder with a shell-safe value such as
subnet=SUBNET_NAME, while preserving the command’s intended argument structure.
🪄 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: 21c53b57-8a6b-4ef0-8228-c5109caf2cec
📒 Files selected for processing (8)
examples/README.mdexamples/catalog-items/ocp-4-20-ai-maas-cluster.yamlexamples/catalog-items/ocp-4-20-nico-baremetal-cluster.yamlexamples/catalog-items/ocp-4-20-openshift-ai-cluster.yamlexamples/catalog-items/ocp-ci-cluster.yamlexamples/catalog-items/simple-ocp-4-17-cluster.yamlexamples/catalog-items/windows-11-vm.yamlexamples/catalog-items/windows-server-vm.yaml
…aults applyFieldDefinitions receives the spec proto directly, so paths should be relative to spec (e.g. network.pod_cidr not spec.network.pod_cidr). The spec. prefix creates phantom keys that are silently ignored during unmarshaling. Also add default CIDR values (10.128.0.0/14, 172.30.0.0/16) to all cluster catalog items so they work with osac create --catalog-item without extra flags, and document catalog item usage in the README. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Daniel Erez <danielerez@gmail.com>
40f226a to
86c626b
Compare
|
/lgtm |
The --network-attachment flag takes a fulfillment subnet ID, not a name. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Daniel Erez <danielerez@gmail.com>
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)
examples/README.md (1)
1-6: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd the required Red Hat AI-assistance commit trailer.
This change documents AI-generated/AI-assisted content, but the commit is missing the required
Assisted-byorGenerated-by: Red Hattrailer. Add it and avoid AI-relatedCo-Authored-Byattributions.🤖 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 `@examples/README.md` around lines 1 - 6, Add the required Red Hat AI-assistance commit trailer using either Assisted-by or Generated-by: Red Hat, and remove any AI-related Co-Authored-By attribution if present. Keep the examples README content unchanged.Source: Coding guidelines
🤖 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 `@examples/README.md`:
- Around line 1-6: Add the required Red Hat AI-assistance commit trailer using
either Assisted-by or Generated-by: Red Hat, and remove any AI-related
Co-Authored-By attribution if present. Keep the examples README content
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cedc7199-2e8d-46aa-b02a-e6944f9d1c64
📒 Files selected for processing (8)
examples/README.mdexamples/catalog-items/ocp-4-20-ai-maas-cluster.yamlexamples/catalog-items/ocp-4-20-nico-baremetal-cluster.yamlexamples/catalog-items/ocp-4-20-openshift-ai-cluster.yamlexamples/catalog-items/ocp-ci-cluster.yamlexamples/catalog-items/simple-ocp-4-17-cluster.yamlexamples/catalog-items/windows-11-vm.yamlexamples/catalog-items/windows-server-vm.yaml
|
/retest |
|
Re-triggered failed runs:
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: CrystalChun, danielerez 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 |
d1268ba
into
osac-project:main
This branch was previously deployed
4 participants
Add example YAML files for catalog items with a README explaining how to use
osac create -fto load them via the private API.Cluster catalog items:
Compute instance (VM) catalog items:
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit
Documentation
osac create -fwith example OSAC YAML files, including login/workflow steps, prerequisites/artifacts, example directory layout, expected YAML schema/fields, and a note that creation is not idempotent.New Examples