OSAC-702: add private CRUD servers for catalog items - #520
openshift-merge-bot[bot] merged 1 commit into
Conversation
|
@tzvatot: This pull request references OSAC-58 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. |
WalkthroughThis PR adds two new private gRPC servers for managing catalog items: one for cluster catalog items and one for compute instance catalog items. The implementation extends the generic server event handling to recognize the new payload types, then introduces parallel server implementations using a consistent builder pattern with full CRUD operations (List, Get, Create, Update, Delete, Signal). Each server is accompanied by comprehensive test coverage validating construction constraints, persistence, filtering, limit handling, and field masking. Finally, both servers are registered during startup in both the gRPC service and REST gateway layers. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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 (2)
internal/servers/private_compute_instance_catalog_items_server.go (1)
74-82: 💤 Low valueConsider validating attributionLogic explicitly for consistency.
The
Build()method explicitly validatesloggerandtenancyLogic, but does not validateattributionLogiceven though it's passed to theGenericServerbuilder (line 88) which requires it. While the validation will still occur inGenericServer.Build()and the error will propagate correctly, explicitly checking all required dependencies here would be more consistent and provide clearer error context.♻️ Suggested explicit validation
if b.logger == nil { err = errors.New("logger is mandatory") return } + if b.attributionLogic == nil { + err = errors.New("attribution logic is mandatory") + return + } if b.tenancyLogic == nil { err = errors.New("tenancy logic is mandatory") return }🤖 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/private_compute_instance_catalog_items_server.go` around lines 74 - 82, The Build() method on PrivateComputeInstanceCatalogItemsServerBuilder currently validates logger and tenancyLogic but omits attributionLogic; add an explicit nil-check for b.attributionLogic in PrivateComputeInstanceCatalogItemsServerBuilder.Build(), returning an error like "attribution logic is mandatory" when nil so callers get a clear, immediate message instead of relying on GenericServer.Build() to catch it; update any related tests or callers if they expect the earlier error path.internal/servers/private_cluster_catalog_items_server.go (1)
74-82: 💤 Low valueConsider validating attributionLogic explicitly for consistency.
The
Build()method explicitly validatesloggerandtenancyLogic, but does not validateattributionLogiceven though it's passed to theGenericServerbuilder (line 88) which requires it. While the validation will still occur inGenericServer.Build()and the error will propagate correctly, explicitly checking all required dependencies here would be more consistent and provide clearer error context.♻️ Suggested explicit validation
if b.logger == nil { err = errors.New("logger is mandatory") return } + if b.attributionLogic == nil { + err = errors.New("attribution logic is mandatory") + return + } if b.tenancyLogic == nil { err = errors.New("tenancy logic is mandatory") return }🤖 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/private_cluster_catalog_items_server.go` around lines 74 - 82, The Build() method of PrivateClusterCatalogItemsServerBuilder currently validates logger and tenancyLogic but omits attributionLogic; add an explicit nil check for attributionLogic in PrivateClusterCatalogItemsServerBuilder.Build() and return a clear error (e.g., "attribution logic is mandatory") if nil so callers get immediate, specific feedback before delegating to GenericServer.Build(); reference the builder struct and its attributionLogic field and keep the error messaging consistent with the existing checks for logger and tenancyLogic.
🤖 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/servers/private_cluster_catalog_items_server.go`:
- Around line 74-82: The Build() method of
PrivateClusterCatalogItemsServerBuilder currently validates logger and
tenancyLogic but omits attributionLogic; add an explicit nil check for
attributionLogic in PrivateClusterCatalogItemsServerBuilder.Build() and return a
clear error (e.g., "attribution logic is mandatory") if nil so callers get
immediate, specific feedback before delegating to GenericServer.Build();
reference the builder struct and its attributionLogic field and keep the error
messaging consistent with the existing checks for logger and tenancyLogic.
In `@internal/servers/private_compute_instance_catalog_items_server.go`:
- Around line 74-82: The Build() method on
PrivateComputeInstanceCatalogItemsServerBuilder currently validates logger and
tenancyLogic but omits attributionLogic; add an explicit nil-check for
b.attributionLogic in PrivateComputeInstanceCatalogItemsServerBuilder.Build(),
returning an error like "attribution logic is mandatory" when nil so callers get
a clear, immediate message instead of relying on GenericServer.Build() to catch
it; update any related tests or callers if they expect the earlier error path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80b4b432-c826-4b3e-bbfb-9eb9bc55bf20
📒 Files selected for processing (7)
internal/cmd/service/start/grpcserver/start_grpc_server_cmd.gointernal/cmd/service/start/restgateway/start_rest_gateway_cmd.gointernal/servers/generic_server.gointernal/servers/private_cluster_catalog_items_server.gointernal/servers/private_cluster_catalog_items_server_test.gointernal/servers/private_compute_instance_catalog_items_server.gointernal/servers/private_compute_instance_catalog_items_server_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, 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 |
08c1697 to
1e56d3f
Compare
|
New changes are detected. LGTM label has been removed. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
1e56d3f to
d6b1956
Compare
|
New changes are detected. LGTM label has been removed. |
- Add private ClusterCatalogItems and ComputeInstanceCatalogItems servers using GenericServer pattern (full CRUD + Signal) - Register both servers in gRPC startup and REST gateway - Add setPayload switch cases for event notification - Add unit tests for both servers (builder validation + CRUD behavior + FieldDefinition round-trip persistence) Generated with [Claude Code](https://claude.com/claude-code)
d6b1956 to
4e265aa
Compare
|
/retest ci/prow/e2e-vmaas |
|
/test e2e-vmaas |
tzvatot
changed the title
|
@tzvatot: This pull request references OSAC-702 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 story 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. |
3 participants
Summary
ClusterCatalogItemsServerandComputeInstanceCatalogItemsServerusing the GenericServer pattern (full CRUD + Signal)setPayloadswitch cases for catalog item event notificationsJIRA: OSAC-58 / OSAC-702
Depends on: #517 (merged)
Validations
buf lint— cleangofmt -s -l .— no formatting issuesgo build ./...— compiles cleanginkgo run -r internal— 52 test suites passed, 0 failuresTest plan
publishedfieldGenerated with Claude Code
Summary by CodeRabbit
New Features
Tests