HYPERFLEET-1154 - feat: Adapter status endpoints for generic resources#284
HYPERFLEET-1154 - feat: Adapter status endpoints for generic resources#284tirthct wants to merge 7 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughAdapter-status validation was centralized for cluster and node-pool services. Resource processing now locks resources, persists adapter statuses, recomputes conditions, and performs hard-delete cleanup with condition-row deletion. New status list/create handlers and Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant StatusHandler
participant ResourceService
participant AdapterStatusDao
participant ResourceConditionDao
Client->>StatusHandler: PUT adapter status
StatusHandler->>ResourceService: ProcessAdapterStatus
ResourceService->>AdapterStatusDao: lock/load/upsert statuses
ResourceService->>ResourceConditionDao: recompute or delete conditions
ResourceService-->>StatusHandler: status or discard
StatusHandler-->>Client: 201, 204, 400, or 404
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 2133 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: pkg/dao plugins/resources | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
pkg/services/adapter_status_validation_test.go (1)
80-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUntested accept branch: equal generation with a newer observed time.
The stale-observed-time discard (
incomingObs.Before(prevObs)) is covered, but the complementary accept path — sameObservedGeneration, non-zero newerLastReportTime,existingStatus != nil— is never asserted. That pass-through branch in the helper (equal-gen block that must NOT discard) is a live path adapters hit on every steady-state re-report. Add a case asserting conditions non-nil and trigger true for a newer timestamp against an existing status.As per path instructions: "New exported functions and critical logic paths SHOULD have tests" / "Error paths SHOULD be tested, not just happy paths" (TEST-01).
🤖 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 `@pkg/services/adapter_status_validation_test.go` around lines 80 - 92, The existing test covers the stale observed-time discard path in validateAndClassifyAdapterStatus, but not the equal-generation accept path when the incoming LastReportTime is newer and existingStatus is non-nil. Add a test near TestValidateAndClassify_StaleObservedTime_Discards that uses adapterStatusWithGenAndTime with the same generation and a newer timestamp than the existing status, then assert conditions is non-nil, trigger is true, and err is nil to exercise the non-discard branch in the equal-gen block.Source: Path instructions
test/integration/resource_status_test.go (1)
1-206: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing integration coverage for the hard-delete-via-status flow.
None of these tests exercise PUT-status against a soft-deleted resource to verify
tryHardDeleteResourcetriggers via the HTTP path. This is the scenario most likely to be impacted by the existence-check ordering flagged inpkg/handlers/resource_status_handler.go— an integration test soft-deleting a resource, then PUTing aFinalized=Truestatus and asserting eventual removal, would directly validate (or falsify) that concern.🤖 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 `@test/integration/resource_status_test.go` around lines 1 - 206, Add integration coverage for the hard-delete-via-status path by extending resource_status_test.go with a case that creates a resource, soft-deletes it, then PUTs a status with Finalized=True through the ResourceStatus handler HTTP route. Use the existing helpers like createTestChannelForStatus, setupResourceTest, and newAdapterStatusRequest to locate the flow, and assert that tryHardDeleteResource is triggered by verifying the resource is eventually removed or returns not found after the status update.pkg/handlers/resource_status_handler.go (1)
73-82: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant existence check on the top-level
Createpath.
ProcessAdapterStatusalready resolves the resource by kind+id viaGetForUpdateand returns aNotFoundServiceErroron failure (handleGetError). The extrah.resourceService.Get(ctx, h.descriptor.Kind, id)call adds a second DB round-trip on every adapter status write (unlikeCreateByOwner, where the equivalentGetByOwnercall is not redundant since it also enforces ownership).♻️ Suggested simplification
Action: func() (interface{}, *errors.ServiceError) { ctx := r.Context() id := mux.Vars(r)["id"] - - if _, err := h.resourceService.Get(ctx, h.descriptor.Kind, id); err != nil { - return nil, err - } - return h.processStatus(ctx, id, &req) },🤖 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 `@pkg/handlers/resource_status_handler.go` around lines 73 - 82, Remove the redundant existence check in the top-level Create path inside the `Action` handler: `ProcessAdapterStatus` already performs the resource lookup through `GetForUpdate` and handles not-found via `handleGetError`. Delete the extra `h.resourceService.Get(ctx, h.descriptor.Kind, id)` call and let `h.processStatus(ctx, id, &req)` own the resource resolution, keeping the `CreateByOwner` flow unchanged since its `GetByOwner` check is still needed for ownership validation.pkg/handlers/resource_status_handler_test.go (1)
1-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for
ListByOwner/CreateByOwner.Only
List/Createare unit-tested here; the ownership-scoped variants are new exported methods and are only exercised indirectly by the integration test. A unit test mockingGetByOwnerfailure (wrong parent) would directly verify the ownership-check branch. As per path instructions,**/*_test.go: "New exported functions and critical logic paths SHOULD have tests."🤖 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 `@pkg/handlers/resource_status_handler_test.go` around lines 1 - 225, Add unit tests for the new ownership-scoped handlers, since only List and Create are covered now. In resource_status_handler_test.go, add cases for ResourceStatusHandler.ListByOwner and ResourceStatusHandler.CreateByOwner that mock ResourceService.GetByOwner and cover the ownership-check failure path when the parent is wrong, plus a success path if needed. Use the existing helper newTestResourceStatusHandler and the method names ListByOwner/CreateByOwner to locate and exercise the new exported logic directly.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.
Nitpick comments:
In `@pkg/handlers/resource_status_handler_test.go`:
- Around line 1-225: Add unit tests for the new ownership-scoped handlers, since
only List and Create are covered now. In resource_status_handler_test.go, add
cases for ResourceStatusHandler.ListByOwner and
ResourceStatusHandler.CreateByOwner that mock ResourceService.GetByOwner and
cover the ownership-check failure path when the parent is wrong, plus a success
path if needed. Use the existing helper newTestResourceStatusHandler and the
method names ListByOwner/CreateByOwner to locate and exercise the new exported
logic directly.
In `@pkg/handlers/resource_status_handler.go`:
- Around line 73-82: Remove the redundant existence check in the top-level
Create path inside the `Action` handler: `ProcessAdapterStatus` already performs
the resource lookup through `GetForUpdate` and handles not-found via
`handleGetError`. Delete the extra `h.resourceService.Get(ctx,
h.descriptor.Kind, id)` call and let `h.processStatus(ctx, id, &req)` own the
resource resolution, keeping the `CreateByOwner` flow unchanged since its
`GetByOwner` check is still needed for ownership validation.
In `@pkg/services/adapter_status_validation_test.go`:
- Around line 80-92: The existing test covers the stale observed-time discard
path in validateAndClassifyAdapterStatus, but not the equal-generation accept
path when the incoming LastReportTime is newer and existingStatus is non-nil.
Add a test near TestValidateAndClassify_StaleObservedTime_Discards that uses
adapterStatusWithGenAndTime with the same generation and a newer timestamp than
the existing status, then assert conditions is non-nil, trigger is true, and err
is nil to exercise the non-discard branch in the equal-gen block.
In `@test/integration/resource_status_test.go`:
- Around line 1-206: Add integration coverage for the hard-delete-via-status
path by extending resource_status_test.go with a case that creates a resource,
soft-deletes it, then PUTs a status with Finalized=True through the
ResourceStatus handler HTTP route. Use the existing helpers like
createTestChannelForStatus, setupResourceTest, and newAdapterStatusRequest to
locate the flow, and assert that tryHardDeleteResource is triggered by verifying
the resource is eventually removed or returns not found after the status update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: cbceb04c-1233-4e9b-afaf-58183e98e2ab
📒 Files selected for processing (14)
pkg/dao/resource.gopkg/dao/resource_condition.gopkg/handlers/resource_status_handler.gopkg/handlers/resource_status_handler_test.gopkg/services/adapter_status_validation.gopkg/services/adapter_status_validation_test.gopkg/services/cluster.gopkg/services/node_pool.gopkg/services/resource.gopkg/services/resource_test.goplugins/entities/plugin.goplugins/entities/plugin_test.goplugins/resources/plugin.gotest/integration/resource_status_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
|
/retest |
1 similar comment
|
/retest |
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 `@pkg/handlers/resource_status_handler.go`:
- Around line 157-159: The client-facing error messages in
resource_status_handler.go are leaking internal details by formatting presErr
into GeneralError, which then gets returned via AsProblemDetails(). Update the
error handling in the adapter status presentation and ConvertAdapterStatus paths
so the returned GeneralError uses a generic message without the raw wrapped
error, and keep the detailed presErr only in server-side logging. Apply the same
fix to all repeated occurrences in this file so no internal presenter/conversion
details reach HTTP responses.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f7d716d6-ddda-47d0-bda6-555d3e533a9d
📒 Files selected for processing (14)
pkg/dao/resource.gopkg/dao/resource_condition.gopkg/handlers/resource_status_handler.gopkg/handlers/resource_status_handler_test.gopkg/services/adapter_status_validation.gopkg/services/adapter_status_validation_test.gopkg/services/cluster.gopkg/services/node_pool.gopkg/services/resource.gopkg/services/resource_test.goplugins/entities/plugin.goplugins/entities/plugin_test.goplugins/resources/plugin.gotest/integration/resource_status_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (13)
- pkg/dao/resource.go
- pkg/services/node_pool.go
- plugins/entities/plugin.go
- plugins/resources/plugin.go
- test/integration/resource_status_test.go
- pkg/services/cluster.go
- pkg/dao/resource_condition.go
- pkg/services/adapter_status_validation_test.go
- pkg/services/adapter_status_validation.go
- pkg/handlers/resource_status_handler_test.go
- plugins/entities/plugin_test.go
- pkg/services/resource.go
- pkg/services/resource_test.go
| // validateAndClassifyNodePool performs all stateless validation and discard-rule checks on an | ||
| // incoming adapter status for a node pool. Returns the parsed conditions and whether aggregation | ||
| // should be triggered. Returns (nil, false, nil) when the update should be silently discarded. | ||
| // validateAndClassifyNodePool delegates to the shared validateAndClassifyAdapterStatus | ||
| // with nodepool-specific logger context. |
There was a problem hiding this comment.
Tip
nit — non-blocking suggestion
Category: Inconsistency
The old doc comment (lines 417-419) still describes the inline validation logic that was extracted into validateAndClassifyAdapterStatus. In cluster.go the old comment was replaced, but here it was kept alongside the new delegation note. This makes the comment block self-contradictory — the method is now a one-liner.
To match how cluster.go:validateAndClassify was handled, replace the full comment block:
| // validateAndClassifyNodePool performs all stateless validation and discard-rule checks on an | |
| // incoming adapter status for a node pool. Returns the parsed conditions and whether aggregation | |
| // should be triggered. Returns (nil, false, nil) when the update should be silently discarded. | |
| // validateAndClassifyNodePool delegates to the shared validateAndClassifyAdapterStatus | |
| // with nodepool-specific logger context. | |
| // validateAndClassifyNodePool delegates to the shared validateAndClassifyAdapterStatus | |
| // with nodepool-specific logger context. |
| // whose children haven't finished their own reconciliation. | ||
| hasChildResources := false | ||
| if resource.DeletedTime != nil { | ||
| for _, child := range registry.ChildrenOf(resource.Kind) { |
There was a problem hiding this comment.
two functions, recomputeAndSaveResourceConditions and tryHardDeleteResource, have the same block. worth doing this one time.
There was a problem hiding this comment.
nit:
thanks, i see that this block was extracted, but recomputeAndSaveResourceConditions is called every time, which means if resource is soft-deleted, we're making additional db requests (one in tryhardelete, other in recompute)
There was a problem hiding this comment.
please remove this guard.
and add before resourceDao.Delete
s.adapterStatusDao.DeleteByResource(ctx, resource.Kind, resource.ID)
s.resourceConditionDao.DeleteByResource(ctx, resource.ID)
And please remove TestAllGenericDescriptors_HaveNoRequiredAdapters and TestGenericDescriptors_HaveNoRequiredAdapters
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/handlers/resource_status_handler_test.go (1)
297-342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing test for
ProcessAdapterStatusreturning a real error (not just thenil,nildiscard case).
TestRootResourceHandler_CreateStatus_HappyPathand_Discarded_Returns204cover success and silent-discard, but no test asserts the handler correctly surfaces a*errors.ServiceErrorfromProcessAdapterStatus(e.g. a validation or DB failure). As per path instructions, "Error paths SHOULD be tested, not just happy paths."func TestRootResourceHandler_CreateStatus_ServiceError(t *testing.T) { RegisterTestingT(t) ctrl := gomock.NewController(t) handler, mockResourceSvc, _ := newTestRootResourceHandler(ctrl) resource := &api.Resource{Kind: "Channel"} resource.ID = testChannelID mockResourceSvc.EXPECT().GetByID(gomock.Any(), testChannelID).Return(resource, nil) mockResourceSvc.EXPECT().ProcessAdapterStatus( gomock.Any(), "Channel", testChannelID, gomock.Any(), ).Return(nil, errors.GeneralError("boom")) // ...build request, PUT, assert 500 }Want me to open a ticket to track this?
🤖 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 `@pkg/handlers/resource_status_handler_test.go` around lines 297 - 342, Add a test in TestRootResourceHandler_CreateStatus coverage for the real error path from ProcessAdapterStatus: mock GetByID to return the resource, have ProcessAdapterStatus return a non-nil *errors.ServiceError such as errors.GeneralError("boom"), then build the PUT request and assert CreateStatus maps that failure to the expected HTTP 500 response instead of only covering the happy path and discarded nil,nil case.Source: Path instructions
pkg/handlers/root_resource_handler.go (1)
197-287: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract the shared status list/create flow
RootResourceHandlerduplicatesResourceStatusHandler’s paginate → present and convert → process → present logic. Move those pieces into shared helpers and pass the resolved kind/resource in from the caller so the status paths stay in sync.🤖 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 `@pkg/handlers/root_resource_handler.go` around lines 197 - 287, RootResourceHandler duplicates the same status list/create pipeline already used by ResourceStatusHandler. Extract the shared paginate → present and convert → process → present logic into reusable helpers, then have ListStatuses and CreateStatus call those helpers with the resolved resource kind and ID from h.service.GetByID so both status paths stay aligned. Keep the presentation/error handling centralized in the shared functions and preserve the existing logger/presenter behavior.
🤖 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 `@pkg/handlers/resource_status_handler_test.go`:
- Around line 297-342: Add a test in TestRootResourceHandler_CreateStatus
coverage for the real error path from ProcessAdapterStatus: mock GetByID to
return the resource, have ProcessAdapterStatus return a non-nil
*errors.ServiceError such as errors.GeneralError("boom"), then build the PUT
request and assert CreateStatus maps that failure to the expected HTTP 500
response instead of only covering the happy path and discarded nil,nil case.
In `@pkg/handlers/root_resource_handler.go`:
- Around line 197-287: RootResourceHandler duplicates the same status
list/create pipeline already used by ResourceStatusHandler. Extract the shared
paginate → present and convert → process → present logic into reusable helpers,
then have ListStatuses and CreateStatus call those helpers with the resolved
resource kind and ID from h.service.GetByID so both status paths stay aligned.
Keep the presentation/error handling centralized in the shared functions and
preserve the existing logger/presenter behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 4d6be611-bf41-4a72-912c-1ebba95d7304
📒 Files selected for processing (4)
pkg/handlers/resource_status_handler_test.gopkg/handlers/root_resource_handler.goplugins/entities/plugin.goplugins/entities/plugin_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/entities/plugin.go
- plugins/entities/plugin_test.go
3 participants
Summary
Adds
GET/PUT /{plural}/{id}/statusesendpoints for all config-driven generic resources(Channel, Version, WifConfig, and future types). Adapters can now report status on generic
resources and trigger aggregated condition computation, completing the adapter status
pipeline for the generic resource layer.
RequiredAdaptersget full adapter lifecycle (soft-delete → finalization → hard-delete)RequiredAdaptersstill accept status reports and aggregate from whatever adapters reportWhat changed
Shared validation — Extracted
validateAndClassifyAdapterStatusfrom the duplicatedlogic in
ClusterService.validateAndClassifyandNodePoolService.validateAndClassifyNodePool(~150 lines of duplication eliminated). Both typed services now delegate to the shared function.
ProcessAdapterStatuson ResourceService — Same 4-DB-call pattern as Cluster/NodePool:lock resource → fetch all adapter statuses → validate + upsert → aggregate conditions.
Key difference: writes conditions to
resource_conditionstable (not JSONB column).ResourceStatusHandler— Generic handler withList/Create/ListByOwner/CreateByOwner,matching
ClusterStatusHandlerpattern. Registered for all entities inplugins/entities/plugin.go.Hard-delete after finalization —
tryHardDeleteResourcepermanently removes asoft-deleted resource when all required adapters report
Finalized=Trueand no children(active or soft-deleted) remain. Cleans up adapter statuses and conditions before removing
the resource row.
DAO updates —
GetForUpdatenow preloadsConditions(needed for aggregation diff).Added
DeleteByResourcetoResourceConditionDaofor hard-delete cleanup.Files
pkg/services/adapter_status_validation.gopkg/services/resource.gopkg/services/cluster.gopkg/services/node_pool.gopkg/handlers/resource_status_handler.goplugins/entities/plugin.goplugins/resources/plugin.gopkg/dao/resource.gopkg/dao/resource_condition.goCondition storage: Cluster vs Generic Resources
resource_conditionstableClusterDao.SaveStatusConditions(id, []byte)ResourceConditionDao.UpdateConditions(id, []cond)Preload("Conditions")on Get/GetForUpdateTest plan
make test— 1343 unit tests passmake test-integration— requires database