HYPERFLEET-1155 - feat: add force-delete endpoint for generic resources#266
Open
kuudori wants to merge 1 commit into
Open
Conversation
|
[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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds force-delete endpoints for resource handlers and registers them for Channel, Version, and WifConfig. The service layer adds ForceDelete with kind validation, finalizing-state checks, recursive child deletion, required-adapter rejection, and hard delete behavior. Tests cover handler responses, service outcomes, and integration behavior. CWE-284 and CWE-862 apply to the exposed destructive flow. Sequence Diagram(s)sequenceDiagram
participant Client
participant ResourceHandler
participant ResourceService
participant resourceDao
Client->>ResourceHandler: POST /{id}/force-delete
ResourceHandler->>ResourceHandler: validate reason
ResourceHandler->>ResourceService: ForceDelete(ctx, kind, id, reason)
ResourceService->>resourceDao: lock and load resource
ResourceService->>resourceDao: delete resource tree
ResourceHandler-->>Client: 204 or mapped error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 3 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 653 lines (>500) | +2 |
| Sensitive paths | none | +0 |
| Test coverage | Missing tests for: plugins/channels plugins/versions plugins/wifconfigs | +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.
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 `@pkg/services/resource_test.go`:
- Around line 1318-1328: The tripwire test is only validating local fixtures
because setupTestDescriptors() rebuilds the registry, so it never checks real
plugin descriptors. Move the RequiredAdapters assertion out of
TestAllGenericDescriptors_HaveNoRequiredAdapters and into a test that loads the
actual generic-resource plugins, then iterate registry.All() to verify the
registered descriptors from the real registry. Keep the existing Expect check
and failure message, but ensure it runs against the production plugin
registration path rather than the test fixture registry.
In `@test/integration/resource_force_delete_test.go`:
- Around line 110-139: The NestedVersionForceDelete subtest in the resource
force-delete integration test only verifies that the Version identified by
versionID is removed, but it does not confirm that the parent channel remains
intact. Update this test around the existing ForceDelete and checkResourceCount
assertions to also assert that channel.ID still exists after the nested version
delete, using the existing helpers in setupResourceTest, createChannel, and
checkResourceCount so the parent scope is explicitly validated.
- Around line 22-141: The current `TestResourceForceDelete` suite only calls
`svc.ForceDelete`, so it misses regressions in
`pkg/handlers/resource_handler.go` and the route/plugin wiring for `POST
/{plural}/{id}/force-delete` and the nested owner-scoped force-delete path.
Update these subtests to use the integration HTTP setup from
`test.RegisterIntegration(t)` (helper, client) with Resty and Gomega, send real
requests through the handler routes, and assert on response status/body plus
`reason` validation instead of only checking `ServiceError.HTTPCode`. Keep the
DB cleanup assertions, but locate the tests by `TestResourceForceDelete`,
`setupResourceTest`, and `ForceDelete` when refactoring.
🪄 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: cd5c2875-03f9-424c-a4fe-6dd27eb0ccdd
📒 Files selected for processing (8)
pkg/handlers/resource_handler.gopkg/handlers/resource_handler_test.gopkg/services/resource.gopkg/services/resource_test.goplugins/channels/plugin.goplugins/versions/plugin.goplugins/wifconfigs/plugin.gotest/integration/resource_force_delete_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)
Comment on lines
+1318
to
+1328
| func TestAllGenericDescriptors_HaveNoRequiredAdapters(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| setupTestDescriptors() | ||
|
|
||
| for _, d := range registry.All() { | ||
| Expect(d.RequiredAdapters).To(BeEmpty(), | ||
| "Descriptor %q has RequiredAdapters=%v. "+ | ||
| "ForceDelete does not yet handle adapter_status cleanup. "+ | ||
| "See HYPERFLEET-1154.", d.Kind, d.RequiredAdapters) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
This tripwire never checks the real descriptor registry.
setupTestDescriptors() wipes the registry and repopulates only local fixtures, so this test will still pass if an actual plugin descriptor later adds RequiredAdapters. That misses the PR’s fail-loud CI invariant entirely. Move this assertion to a test that imports the real generic-resource plugins and inspects their registered descriptors instead of rebuilding the registry here.
🤖 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/resource_test.go` around lines 1318 - 1328, The tripwire test is
only validating local fixtures because setupTestDescriptors() rebuilds the
registry, so it never checks real plugin descriptors. Move the RequiredAdapters
assertion out of TestAllGenericDescriptors_HaveNoRequiredAdapters and into a
test that loads the actual generic-resource plugins, then iterate registry.All()
to verify the registered descriptors from the real registry. Keep the existing
Expect check and failure message, but ensure it runs against the production
plugin registration path rather than the test fixture registry.
Add POST /{plural}/{id}/force-delete to ResourceHandler for all generic
entity types (channels, versions, wifconfigs). The endpoint bypasses
OnParentDelete policies and recursively hard-deletes the entire ownership
tree for resources in Finalizing state (deleted_time IS NOT NULL).
Key design decisions per ADR-0013 (DB-only force-delete):
- Requires Finalizing state and a reason field in the request body
- Bottom-up hard-delete ordering per ADR-0012
- Fail-loud guard if RequiredAdapters non-empty (tripwire for HYPERFLEET-1154)
- MarkForRollback on error prevents partial tree deletion from committing
- Audit log with structured fields before each resource deletion
Includes unit tests (handler + service + tripwire) and integration tests
confirming force-delete succeeds where normal DELETE returns 409 due to
Restrict policy.
kuudori
force-pushed
the
HYPERFLEET-1155-feat-force-delete-generic-resources
branch
from
8779683 to
c0d96a5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
POST /{plural}/{id}/force-deletetoResourceHandlerfor all generic entity types (channels, versions, wifconfigs)Design
Per ADR-0013 (DB-only force-delete) and ADR-0012 (bottom-up deletion ordering):
deleted_time IS NOT NULL(409 otherwise)validateNotEmpty+validateMaxLengthforceDeleteResourceTreewalks all children regardless of OnParentDelete policy, hard-deletes bottom-upMarkForRollbackon any error prevents partial commitsTestAllGenericDescriptors_HaveNoRequiredAdaptersbreaks CI when assumption changesTest plan
TestAllGenericDescriptors_HaveNoRequiredAdaptersmake verify-allpasses (1322 tests)make test-integrationpasses (force-delete scenarios)