Conversation
|
@vladikr: This pull request references OSAC-1419 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 epic 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. |
|
Hi @vladikr. Thanks for your PR. I'm waiting for a osac-project member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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 kubernetes-sigs/prow repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds an idempotent ChangesCatalog Items Seeding and Integration Tests
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over CLI,API: seed-catalog-items idempotent seeding flow
CLI->>run: Execute seed-catalog-items (--dry-run, --api-url, --token)
run->>run: Validate inputs, create gRPC connection
run->>API: seedClusterCatalogItems()
loop Per predefined cluster item
run->>API: ListClusterCatalogItems
API-->>run: existing items by name
alt Name found in list
run->>run: skip (already exists)
else --dry-run enabled
run->>run: would create (no gRPC call)
else Create
run->>API: CreateClusterCatalogItem
alt AlreadyExists error
API-->>run: skipped (logged as race)
else Success
API-->>run: created
end
end
end
run->>API: seedComputeCatalogItems()
run->>API: Create compute items (same idempotency logic)
run-->>CLI: Print aggregate created/skipped totals
end
rect rgba(144, 238, 144, 0.5)
Note over Test,PublicAPI: Cluster catalog item test validations
Test->>PrivateAPI: Create cluster template (prerequisite)
Test->>PrivateAPI: CreateClusterCatalogItem (published=true)
PrivateAPI-->>Test: Item response with echoed field definitions
Test->>Test: Assert field attributes (path, display_name, editable, default, schema)
Test->>PublicAPI: ListClusterCatalogItems
PublicAPI-->>Test: Published item appears in listing
Test->>PrivateAPI: CreateClusterCatalogItem (published=false)
Test->>PublicAPI: ListClusterCatalogItems
PublicAPI-->>Test: Unpublished item absent from listing
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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.
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 `@hack/seed-catalog-items.sh`:
- Around line 49-53: In the `list_existing()` function, the jq filter is using
the incorrect field name `.objects[]` when it should be `.items[]` to match the
actual API response structure defined in the proto. Replace `.objects[]` with
`.items[]` in the jq command. Additionally, replace the generic `|| echo ""`
fallback with proper error handling that distinguishes between actual
API/network errors and empty results, ensuring the script can reliably detect
existing catalog items and maintain idempotency.
- Around line 72-84: The curl command assignment to the response variable exits
the script immediately on failure due to set -euo pipefail, preventing the error
handling branch from executing. Move the curl command directly into the if
condition so that its exit status is properly evaluated and the error handling
in the else branch executes correctly when the API request fails.
In `@it/it_catalog_items_test.go`:
- Around line 117-121: Replace all three occurrences of the GetObjects() method
call with GetItems() in the list response accessor calls. The
publicv1.ClusterCatalogItemsListRequest response object provides GetItems() not
GetObjects() for accessing the catalog items collection. Update each instance
where listResponse.GetObjects() is called to use listResponse.GetItems()
instead.
- Around line 40-48: The BeforeEach function uses context.Background() without a
timeout, which can cause integration tests to hang indefinitely if dependencies
stall. Replace the ctx assignment with context.WithTimeout(context.Background(),
timeoutValue) where timeoutValue is a reasonable timeout like 30 seconds, and
capture the returned cancel function. Add a DeferCleanup(cancel) call
immediately after to ensure proper cleanup of the cancellation when the test
completes.
🪄 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: d5d61a5a-a15d-464c-b588-4eaf0fb5bd5b
📒 Files selected for processing (2)
hack/seed-catalog-items.shit/it_catalog_items_test.go
| curl -sf -H "Authorization: Bearer ${TOKEN}" \ | ||
| "${API_URL}/api/private/v1/${type}?size=1000" 2>/dev/null \ | ||
| | jq -r '.objects[]?.metadata.name // empty' \ | ||
| || echo "" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify the list-response field names used by private APIs.
rg -n 'message (ClusterCatalogItemsListResponse|ComputeInstanceCatalogItemsListResponse)' \
proto/private/osac/private/v1 -A12 -B2Repository: osac-project/fulfillment-service
Length of output: 2848
🏁 Script executed:
cat -n hack/seed-catalog-items.shRepository: osac-project/fulfillment-service
Length of output: 8531
🏁 Script executed:
# Check the structure of catalog item protos to understand the response format
rg -n 'message ClusterCatalogItem|message ComputeInstanceCatalogItem' \
proto/private/osac/private/v1 -A15Repository: osac-project/fulfillment-service
Length of output: 17841
Fix incorrect response field name and add proper error handling to list_existing()
The jq filter uses .objects[] but the API response field is .items per the proto definition. This causes jq to fail on every invocation, making the fallback || echo "" treat all failures (auth, network, or parse errors) identically as "empty catalog". Combined, this breaks idempotency: the script cannot reliably detect existing items, risking duplicate creation.
Suggested fix
list_existing() {
local type=$1
curl -sf -H "Authorization: Bearer ${TOKEN}" \
"${API_URL}/api/private/v1/${type}?size=1000" 2>/dev/null \
- | jq -r '.objects[]?.metadata.name // empty' \
- || echo ""
+ | jq -r '.items[]?.metadata.name // empty'
}
@@
local existing
- existing=$(list_existing "$type")
+ if ! existing=$(list_existing "$type"); then
+ echo " ❌ failed to list existing ${type}" >&2
+ return 1
+ fi🤖 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 `@hack/seed-catalog-items.sh` around lines 49 - 53, In the `list_existing()`
function, the jq filter is using the incorrect field name `.objects[]` when it
should be `.items[]` to match the actual API response structure defined in the
proto. Replace `.objects[]` with `.items[]` in the jq command. Additionally,
replace the generic `|| echo ""` fallback with proper error handling that
distinguishes between actual API/network errors and empty results, ensuring the
script can reliably detect existing catalog items and maintain idempotency.
7797fa9 to
ac9ef16
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hack/seed-catalog-items.sh (1)
49-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not collapse list failures into “empty catalog” results.
At Line 54,
|| echo ""makes auth/network/JSON failures look like valid empty results, socreate_if_missingcan mis-detect existence and attempt unintended creates. Propagate failure fromlist_existing()and fail fast increate_if_missing().Proposed fix
list_existing() { local type=$1 curl -sf -H "Authorization: Bearer ${TOKEN}" \ - "${API_URL}/api/private/v1/${type}?size=1000" 2>/dev/null \ - | jq -r '.items[]?.metadata.name // empty' \ - || echo "" + "${API_URL}/api/private/v1/${type}?size=1000" \ + | jq -r '.items[]?.metadata.name // empty' } @@ - existing=$(list_existing "$type") + if ! existing=$(list_existing "$type"); then + echo " Failed to list existing ${type}" >&2 + return 1 + fiAlso applies to: 64-65
🤖 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 `@hack/seed-catalog-items.sh` around lines 49 - 55, The `|| echo ""` fallback at the end of the pipeline in the `list_existing()` function masks authentication, network, and JSON parsing failures by making them appear as valid empty results, which causes `create_if_missing()` to incorrectly detect non-existence and attempt unintended creates. Remove the `|| echo ""` fallback from the `list_existing()` function to allow errors to propagate and fail fast. Apply the same fix to the other similar pattern mentioned at lines 64-65.
🤖 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 `@it/it_catalog_items_test.go`:
- Around line 258-265: The test assertion on GetFieldDefinitions() is only
verifying the count of field definitions using HaveLen(2) but not validating the
actual content of those definitions, which means regressions in field definition
properties like name, path, default, or validation_schema could be missed.
Replace or supplement the HaveLen(2) assertion with specific assertions that
verify the content of each field definition returned by GetFieldDefinitions(),
ensuring properties like name, path, default values, and validation schemas
match expected values for the compute field definitions.
---
Duplicate comments:
In `@hack/seed-catalog-items.sh`:
- Around line 49-55: The `|| echo ""` fallback at the end of the pipeline in the
`list_existing()` function masks authentication, network, and JSON parsing
failures by making them appear as valid empty results, which causes
`create_if_missing()` to incorrectly detect non-existence and attempt unintended
creates. Remove the `|| echo ""` fallback from the `list_existing()` function to
allow errors to propagate and fail fast. Apply the same fix to the other similar
pattern mentioned at lines 64-65.
🪄 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: d3bce919-00da-4e07-817b-9d8e8cf86c5c
📒 Files selected for processing (2)
hack/seed-catalog-items.shit/it_catalog_items_test.go
| object := response.GetObject() | ||
| Expect(object).ToNot(BeNil()) | ||
| Expect(object.GetMetadata().GetName()).To(Equal(name)) | ||
| Expect(object.GetTitle()).To(Equal("Linux Virtual Machine")) | ||
| Expect(object.GetTemplate()).To(Equal(templateID)) | ||
| Expect(object.GetPublished()).To(BeTrue()) | ||
| Expect(object.GetFieldDefinitions()).To(HaveLen(2)) | ||
| }) |
There was a problem hiding this comment.
Assert compute field-definition content, not only count.
At Line 264, HaveLen(2) alone can still pass if path, default, or validation_schema are persisted incorrectly, so this test can miss regressions on compute field definitions.
Suggested test hardening
object := response.GetObject()
Expect(object).ToNot(BeNil())
Expect(object.GetMetadata().GetName()).To(Equal(name))
Expect(object.GetTitle()).To(Equal("Linux Virtual Machine"))
Expect(object.GetTemplate()).To(Equal(templateID))
Expect(object.GetPublished()).To(BeTrue())
- Expect(object.GetFieldDefinitions()).To(HaveLen(2))
+ fieldDefs := object.GetFieldDefinitions()
+ Expect(fieldDefs).To(HaveLen(2))
+ Expect(fieldDefs[0].GetPath()).To(Equal("spec.cores"))
+ Expect(fieldDefs[0].GetDefault().GetNumberValue()).To(Equal(float64(8)))
+ Expect(fieldDefs[0].GetValidationSchema()).To(ContainSubstring(`"maximum": 64`))
+ Expect(fieldDefs[1].GetPath()).To(Equal("spec.memory_gib"))
+ Expect(fieldDefs[1].GetDefault().GetNumberValue()).To(Equal(float64(64)))
+ Expect(fieldDefs[1].GetValidationSchema()).To(ContainSubstring(`"maximum": 512`))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| object := response.GetObject() | |
| Expect(object).ToNot(BeNil()) | |
| Expect(object.GetMetadata().GetName()).To(Equal(name)) | |
| Expect(object.GetTitle()).To(Equal("Linux Virtual Machine")) | |
| Expect(object.GetTemplate()).To(Equal(templateID)) | |
| Expect(object.GetPublished()).To(BeTrue()) | |
| Expect(object.GetFieldDefinitions()).To(HaveLen(2)) | |
| }) | |
| object := response.GetObject() | |
| Expect(object).ToNot(BeNil()) | |
| Expect(object.GetMetadata().GetName()).To(Equal(name)) | |
| Expect(object.GetTitle()).To(Equal("Linux Virtual Machine")) | |
| Expect(object.GetTemplate()).To(Equal(templateID)) | |
| Expect(object.GetPublished()).To(BeTrue()) | |
| fieldDefs := object.GetFieldDefinitions() | |
| Expect(fieldDefs).To(HaveLen(2)) | |
| Expect(fieldDefs[0].GetPath()).To(Equal("spec.cores")) | |
| Expect(fieldDefs[0].GetDefault().GetNumberValue()).To(Equal(float64(8))) | |
| Expect(fieldDefs[0].GetValidationSchema()).To(ContainSubstring(`"maximum": 64`)) | |
| Expect(fieldDefs[1].GetPath()).To(Equal("spec.memory_gib")) | |
| Expect(fieldDefs[1].GetDefault().GetNumberValue()).To(Equal(float64(64))) | |
| Expect(fieldDefs[1].GetValidationSchema()).To(ContainSubstring(`"maximum": 512`)) | |
| }) |
🤖 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 `@it/it_catalog_items_test.go` around lines 258 - 265, The test assertion on
GetFieldDefinitions() is only verifying the count of field definitions using
HaveLen(2) but not validating the actual content of those definitions, which
means regressions in field definition properties like name, path, default, or
validation_schema could be missed. Replace or supplement the HaveLen(2)
assertion with specific assertions that verify the content of each field
definition returned by GetFieldDefinitions(), ensuring properties like name,
path, default values, and validation schemas match expected values for the
compute field definitions.
ac9ef16 to
0432838
Compare
There was a problem hiding this comment.
Don't add "hacks" or scripts, please. You can add a new sub-command to the existing "osac-dev" binary, written in Go. You can use this as an example: https://github.com/osac-project/fulfillment-service/tree/main/internal/cmd/osac-dev/generate.
There was a problem hiding this comment.
Thanks for the feedback @jhernand
The original request was to quickly unblock the UI team for 0.1 with some hardcoded defaults. A shell script was the fastest way to do that.
I'm happy to implement this properly as a osac-dev subcommand, but that will take more time and would need to be reworked for 0.2.
There was a problem hiding this comment.
If you don't have the time then I'd suggest you just create an "examples/catalog-items" directory and put there YAML files, and a README explaining how to use "osac create -f ..." to add them to the system.
There was a problem hiding this comment.
If you don't have the time then I'd suggest you just create an "examples/catalog-items" directory and put
there YAML files, and a README explaining how to use "osac create -f ..." to add them to the system.
I would stick to this. Let's not over-complicate things. Bonus points for using the examples in e2e tests.
78b60f0 to
0e0b5db
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/README.md`:
- Around line 18-24: The osac create examples using `-f
simple-ocp-4-17-cluster.yaml` and `-f .` are ambiguous about the working
directory context. Add an explicit step before these commands to change into the
examples/catalog-items directory using cd, or alternatively modify the file path
arguments to use repo-root-relative paths like `-f
examples/catalog-items/simple-ocp-4-17-cluster.yaml` and `-f
examples/catalog-items/` so the examples work regardless of the user's current
directory.
- Around line 33-37: The example command using seed-catalog-items includes the
--insecure flag without any warning or context about its usage restrictions. Add
a note or comment in the README immediately before or after the command block
that clearly indicates the --insecure flag is for local development and testing
only, and should be removed when TLS is properly configured in production
environments. This helps developers understand the security implications and
prevents accidental use of insecure configurations.
🪄 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: 9a96bded-1959-4e0b-87bf-fd87d3dd20d3
📒 Files selected for processing (7)
examples/catalog-items/README.mdexamples/catalog-items/linux-vm.yamlexamples/catalog-items/ocp-4-20-nico-baremetal-cluster.yamlexamples/catalog-items/simple-ocp-4-17-cluster.yamlexamples/catalog-items/windows-vm.yamlinternal/cmd/osac-dev/root_cmd.gointernal/cmd/osac-dev/seedcatalogitems/seed_catalog_items_cmd.go
💤 Files with no reviewable changes (2)
- internal/cmd/osac-dev/root_cmd.go
- internal/cmd/osac-dev/seedcatalogitems/seed_catalog_items_cmd.go
| ```bash | ||
| # Create a single catalog item | ||
| osac create -f simple-ocp-4-17-cluster.yaml --private | ||
|
|
||
| # Create all catalog items at once | ||
| osac create -f . --private | ||
| ``` |
There was a problem hiding this comment.
Clarify the working directory for these osac create examples.
These commands assume the current directory is examples/catalog-items; otherwise -f simple-ocp-4-17-cluster.yaml and -f . won’t target the intended files. Add an explicit cd examples/catalog-items step or use repo-root-relative paths in the examples.
🤖 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/README.md` around lines 18 - 24, The osac create
examples using `-f simple-ocp-4-17-cluster.yaml` and `-f .` are ambiguous about
the working directory context. Add an explicit step before these commands to
change into the examples/catalog-items directory using cd, or alternatively
modify the file path arguments to use repo-root-relative paths like `-f
examples/catalog-items/simple-ocp-4-17-cluster.yaml` and `-f
examples/catalog-items/` so the examples work regardless of the user's current
directory.
| go run ./cmd/osac-dev seed-catalog-items \ | ||
| --api-url=fulfillment-api.osac.svc.cluster.local:443 \ | ||
| --token=$(kubectl create token -n osac client) \ | ||
| --insecure | ||
| ``` |
There was a problem hiding this comment.
Scope --insecure to local/dev-only usage in the example.
Please add a short warning that --insecure is only for development environments and should be removed when TLS is properly configured.
🤖 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/README.md` around lines 33 - 37, The example command
using seed-catalog-items includes the --insecure flag without any warning or
context about its usage restrictions. Add a note or comment in the README
immediately before or after the command block that clearly indicates the
--insecure flag is for local development and testing only, and should be removed
when TLS is properly configured in production environments. This helps
developers understand the security implications and prevents accidental use of
insecure configurations.
0e0b5db to
26a1c0f
Compare
|
/ok-to-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, vladikr 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 |
26a1c0f to
0b53677
Compare
|
New changes are detected. LGTM label has been removed. |
|
I removed the "Published compute instance catalog items appear in public API" test. The test was failing with PermissionDenied on the public.v1.ComputeInstanceCatalogItems/List endpoint. I think the issue is a missing OPA authorization policy for public compute instance catalog item listing. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/osac-dev/seedcatalogitems/seed_catalog_items_cmd.go`:
- Around line 87-88: Add request deadlines to prevent indefinite blocking in
gRPC operations. Replace the context creation at line 87 (currently using
context.WithCancel) with context.WithTimeout to set an appropriate timeout
duration for the RPC flow. Apply this timeout context to all gRPC calls in the
file, specifically the List and Create RPC calls at lines 351-352, 374-376,
396-397, and 419-421. Additionally, at line 171 where json.Marshal is called,
handle the error return value instead of ignoring it with an underscore; either
properly handle the error or add a clear comment explaining why the error cannot
occur in that context.
- Around line 171-173: The validationSchema function currently ignores the error
from json.Marshal by using blank identifier assignment, which means
non-serializable values in the map[string]any parameter will silently return an
empty string. Instead of discarding the error, modify the function to properly
handle the json.Marshal error by either returning it as a second return value
from validationSchema or handling it with a panic/log statement. Reference how
json.Marshal errors are handled in similar functions throughout the codebase
(such as in edit_cmd.go and create_cmd.go) to maintain consistency with the
existing error handling patterns.
In `@it/it_catalog_items_test.go`:
- Around line 191-193: The test code dereferences the response object without
first validating that it is not nil, which will cause the test to panic instead
of failing with a clear assertion if the create operation returns a nil
response. Add nil assertions using Expect() before dereferencing response on the
line that calls response.GetObject(), and add another nil assertion for the
object returned before calling object.GetFieldDefinitions(). This ensures that
if either response or the object is nil, the test fails with a meaningful
assertion message rather than a panic.
🪄 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: bfcf17fa-96e3-4385-ac0c-c8ac1fd827fc
📒 Files selected for processing (8)
examples/catalog-items/README.mdexamples/catalog-items/linux-vm.yamlexamples/catalog-items/ocp-4-20-nico-baremetal-cluster.yamlexamples/catalog-items/simple-ocp-4-17-cluster.yamlexamples/catalog-items/windows-vm.yamlinternal/cmd/osac-dev/root_cmd.gointernal/cmd/osac-dev/seedcatalogitems/seed_catalog_items_cmd.goit/it_catalog_items_test.go
0b53677 to
7342956
Compare
90fe496 to
587cb53
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (4)
examples/catalog-items/README.md (2)
35-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit dev-only warning for
--insecure.Line 39 shows
--insecurein the primary example, but the section doesn’t explicitly warn to remove it outside local/dev usage.Suggested fix
go run ./cmd/osac-dev seed-catalog-items \ --api-url=fulfillment-api.osac.svc.cluster.local:443 \ --token=$(kubectl create token -n osac client) \ --insecure
+>
⚠️ --insecureis for local/dev environments only. Remove it when TLS is properly configured.</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/README.mdaround lines 35 - 40, The bash command
example in the catalog-items README shows the --insecure flag without explicitly
warning that it is for local/dev environments only. Add a warning block
immediately after the code example (around line 40) that clearly indicates the
--insecure flag should be removed when TLS is properly configured in non-dev
environments. The warning should use a blockquote format with a warning symbol
to make it visually distinct and emphasize this is a security consideration.</details> <!-- cr-comment:v1:126122d010b412220ea34f28 --> --- `18-27`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Make the `osac create` examples directory-independent.** Line 23 and Line 26 depend on being inside `examples/catalog-items`, which is easy to miss. <details> <summary>Suggested fix</summary> ```diff -# Create a single catalog item -osac create -f simple-ocp-4-17-cluster.yaml +# Create a single catalog item +osac create -f examples/catalog-items/simple-ocp-4-17-cluster.yaml # Create all catalog items at once -osac create -f . +osac create -f examples/catalog-items🤖 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/README.md` around lines 18 - 27, The osac create examples are directory-dependent and assume the user is already inside the examples/catalog-items directory. Update the two osac create command examples to use explicit paths instead of relative paths. For the first command that references simple-ocp-4-17-cluster.yaml, include the full path from the repository root (examples/catalog-items/simple-ocp-4-17-cluster.yaml), and for the second command that uses the current directory (.), replace it with the explicit examples/catalog-items directory path so these examples work regardless of the user's current working directory.internal/cmd/osac-dev/seedcatalogitems/seed_catalog_items_cmd.go (1)
182-186:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTighten CIDR validation to reject invalid IP/prefix values.
Line 185 accepts invalid CIDRs (for example
999.999.999.999/99), so bad network values can be stored and only fail downstream. Use a stricter pattern that enforces octet range0-255and prefix range0-32(and mirror the same schema in the example YAMLs).Suggested fix
func cidrValidationSchema() string { return validationSchema(map[string]any{ "type": "string", - "pattern": `^([0-9]{1,3}\.){3}[0-9]{1,3}/[0-9]{1,2}$`, + "pattern": `^((25[0-5]|2[0-4][0-9]|1?[0-9]?[0-9])\.){3}(25[0-5]|2[0-4][0-9]|1?[0-9]?[0-9])/(3[0-2]|[12]?[0-9])$`, }) }As per path instructions, "Validate at trust boundaries with allow-lists, not deny-lists."
🤖 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/osac-dev/seedcatalogitems/seed_catalog_items_cmd.go` around lines 182 - 186, The cidrValidationSchema function uses a regex pattern that is too permissive and allows invalid CIDR values like 999.999.999.999/99. Replace the current pattern with a stricter regex that enforces octet values in the range 0-255 and prefix length in the range 0-32 for IPv4 CIDR notation. Additionally, ensure that any example YAMLs or other documentation that reference CIDR validation schemas are updated to use the same stricter validation pattern for consistency.Source: Path instructions
it/it_catalog_items_test.go (1)
256-263: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winStrengthen compute field-definition assertions beyond length
Line 262 only checks
HaveLen(2). This can pass even ifpath, defaults, or validation schemas are wrong.Suggested patch
object := response.GetObject() Expect(object).ToNot(BeNil()) Expect(object.GetMetadata().GetName()).To(Equal(name)) Expect(object.GetTitle()).To(Equal("Linux Virtual Machine")) Expect(object.GetTemplate()).To(Equal(templateID)) Expect(object.GetPublished()).To(BeTrue()) - Expect(object.GetFieldDefinitions()).To(HaveLen(2)) + fieldDefs := object.GetFieldDefinitions() + Expect(fieldDefs).To(HaveLen(2)) + Expect(fieldDefs[0].GetPath()).To(Equal("spec.cores")) + Expect(fieldDefs[0].GetDisplayName()).To(Equal("CPU Cores")) + Expect(fieldDefs[0].GetEditable()).To(BeTrue()) + Expect(fieldDefs[0].GetDefault().GetNumberValue()).To(Equal(float64(8))) + Expect(fieldDefs[0].GetValidationSchema()).To(ContainSubstring(`"maximum": 64`)) + Expect(fieldDefs[1].GetPath()).To(Equal("spec.memory_gib")) + Expect(fieldDefs[1].GetDisplayName()).To(Equal("Memory (GiB)")) + Expect(fieldDefs[1].GetEditable()).To(BeTrue()) + Expect(fieldDefs[1].GetDefault().GetNumberValue()).To(Equal(float64(64))) + Expect(fieldDefs[1].GetValidationSchema()).To(ContainSubstring(`"maximum": 512`))🤖 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 `@it/it_catalog_items_test.go` around lines 256 - 263, The assertion on GetFieldDefinitions() in the test only validates the length is 2 but does not verify the actual content or properties of the field definitions themselves. Strengthen this assertion by adding checks for the specific properties of each field definition (such as path, defaults, and validation schemas) in addition to the length check. This ensures the field definitions contain the expected values, not just the correct count, making the test more robust and meaningful.
🤖 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.
Duplicate comments:
In `@examples/catalog-items/README.md`:
- Around line 35-40: The bash command example in the catalog-items README shows
the --insecure flag without explicitly warning that it is for local/dev
environments only. Add a warning block immediately after the code example
(around line 40) that clearly indicates the --insecure flag should be removed
when TLS is properly configured in non-dev environments. The warning should use
a blockquote format with a warning symbol to make it visually distinct and
emphasize this is a security consideration.
- Around line 18-27: The osac create examples are directory-dependent and assume
the user is already inside the examples/catalog-items directory. Update the two
osac create command examples to use explicit paths instead of relative paths.
For the first command that references simple-ocp-4-17-cluster.yaml, include the
full path from the repository root
(examples/catalog-items/simple-ocp-4-17-cluster.yaml), and for the second
command that uses the current directory (.), replace it with the explicit
examples/catalog-items directory path so these examples work regardless of the
user's current working directory.
In `@internal/cmd/osac-dev/seedcatalogitems/seed_catalog_items_cmd.go`:
- Around line 182-186: The cidrValidationSchema function uses a regex pattern
that is too permissive and allows invalid CIDR values like 999.999.999.999/99.
Replace the current pattern with a stricter regex that enforces octet values in
the range 0-255 and prefix length in the range 0-32 for IPv4 CIDR notation.
Additionally, ensure that any example YAMLs or other documentation that
reference CIDR validation schemas are updated to use the same stricter
validation pattern for consistency.
In `@it/it_catalog_items_test.go`:
- Around line 256-263: The assertion on GetFieldDefinitions() in the test only
validates the length is 2 but does not verify the actual content or properties
of the field definitions themselves. Strengthen this assertion by adding checks
for the specific properties of each field definition (such as path, defaults,
and validation schemas) in addition to the length check. This ensures the field
definitions contain the expected values, not just the correct count, making the
test more robust and meaningful.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 0db8d903-69ce-4552-9ad9-fe9f9672c03a
📒 Files selected for processing (8)
examples/catalog-items/README.mdexamples/catalog-items/linux-vm.yamlexamples/catalog-items/ocp-4-20-nico-baremetal-cluster.yamlexamples/catalog-items/simple-ocp-4-17-cluster.yamlexamples/catalog-items/windows-vm.yamlinternal/cmd/osac-dev/root_cmd.gointernal/cmd/osac-dev/seedcatalogitems/seed_catalog_items_cmd.goit/it_catalog_items_test.go
1a6d5c3 to
34022bc
Compare
|
Creating vms from catalogitems with network_attachments from both UI and CLI will always fail with |
| editable: true | ||
| default: 8 | ||
| validation_schema: '{"type":"integer","minimum":1,"maximum":64}' | ||
| - path: memory_gib |
There was a problem hiding this comment.
VM creation was recently changed to use instancetypes instead of cores and memory
#735
Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Vladik Romanovsky <vromanso@redhat.com>
34022bc to
98b47c8
Compare
|
The VM catalog items ( We should pick one of these before merge:
|
|
In addition - for windows VM, I think spec.is_windows should be true |
|
PR needs rebase. DetailsInstructions 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 kubernetes-sigs/prow repository. |
6 participants
Add a new
osac-dev seed-catalog-itemssubcommand that seeds a small set of default catalog items into the fulfillment service.This unblocks the UI team by providing ready-to-use catalog items for development and testing.
Default catalog items created:
The script is idempotent and can be run multiple times safely.
Usage:
Summary by CodeRabbit
Summary
New Features
seed-catalog-itemsCLI command with a--dry-runoption to preview what would be created.Documentation
Tests