feat: Allow partial success results - #400
Conversation
91aa4ef to
e0a2d5b
Compare
nurof3n
changed the title
67276b3 to
1d4c4d4
Compare
|
Ho hum. Can I get an explanation of what this one is for? 🤔 It's not immediately obvious to me sorry! |
@jedevc Yup, so the router can return a 207 on listing or bulk requests when some of the metros returned errors, and it would contain both the results for successful calls and the errors for the others. |
1d4c4d4 to
48a0314
Compare
There was a problem hiding this comment.
Pull request overview
This PR extends the CLI’s multi-metro list/get/delete operations to allow returning partial results when the API returns both a response payload and an error (or when only some items succeed). It introduces shared helpers in internal/cmd/util.go and wires them into several resource commands (instances, volumes, templates, certificates, service groups), aligning with TOOL-1048 and the follow-up to PR #388.
Changes:
- Add shared partial-success helpers and a
PartialResulterror type to preserve successful items while still surfacing failures. - Update list/get handlers across multiple resources to continue processing response payloads even when an API error is present (when data exists).
- Update delete handlers to detect partial deletions and return structured partial failures when possible.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cmd/util.go | Adds partial-success helpers (listGetOpError, deleteOpError), PartialResult, and error-combining utilities. |
| internal/cmd/instances.go | Uses partial-success logic for list/get and returns partial delete results when some instances delete successfully. |
| internal/cmd/instance_templates.go | Uses partial-success logic for template instance list/get/delete operations. |
| internal/cmd/volumes.go | Uses partial-success logic for volume list/get and returns partial delete results when possible. |
| internal/cmd/volume_templates.go | Uses partial-success logic for template volume list/get/delete operations. |
| internal/cmd/certificates.go | Uses partial-success logic for certificate list/get and returns partial delete results when possible. |
| internal/cmd/services.go | Uses partial-success logic for service group list/get; delete path attempts partial handling but currently has dead/unreachable logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
48a0314 to
7aece91
Compare
Makes sense. The bit that I don't get is the new helpers in |
eb97ef3 to
c82a922
Compare
|
@jedevc My bad, I simplified it and now we don't abort processing on partial success. |
c82a922 to
3d464ff
Compare
5c8e9aa to
cdb4605
Compare
jedevc
left a comment
jedevc
left a comment
There was a problem hiding this comment.
Ahh, this is super neat. I like this a lot 🎉
Small comments, good finds all around.
The router can answer a listing or a bulk request with a 207, carrying both the results from the metros that succeeded and the errors from the ones that did not. Abort on such a response and the successful half is thrown away, so nothing is printed at all. Never stop processing a response: collect whatever data came back and join it with the errors, letting the callers decide. The commands already print partial results before returning the error, so this is mostly a matter of not discarding them on the way up. Not-found is the exception: the group helpers already report the refs that were missing, so ignoreNotFound() drops it on ref-based operations to avoid reporting it twice. A listing has no refs to fall back on, so it keeps reporting the error itself. The lookups that surround a mutation only exist to display it, so a partial lookup there is warned about rather than turned into a failure. Waiting is not a mutation: the conditions can only be evaluated against the resources that were looked up, so an incomplete lookup keeps the wait polling instead of letting it succeed early. Signed-off-by: Alex-Andrei Cioc <andrei.cioc@unikraft.io>
Checkpoints, and the history endpoint behind them, landed after this branch was cut, so they still aborted on a 207 and threw away the metros that answered. Same treatment as the rest: read whatever data came back before looking at the error, and hand it up alongside ignoreNotFound() so the group helpers stay in charge of reporting missing refs. Signed-off-by: Justin Chadwell <justin@unikraft.com>
cdb4605 to
89ce48a
Compare
Applied everywhere the patterns appear rather than only at the lines they were raised on. errors.Join already drops nils, so guarding the append with an if in the listing loops buys nothing. Drop the guard and let the join do the work. The partial-lookup warnings talked about metros, which internal/resource has no concept of: it drives images, profiles and config through the same interfaces, and the fan-out is a detail of internal/cmd. They now say what the layer actually knows — which resource type could not be read and what that costs the reader — with the type and keys carried as structured fields rather than left inside the wrapped error. A 207 also has to fail the command. Listing and getting already returned the error after printing, but creating, deleting and editing logged a warning and returned nil, so a script checking the exit code was told everything was fine when half the metros never answered. Return it instead, still only once the output has been written. Bulk remove returned before printing whenever the delete failed, so a partial delete reported neither the refs it removed nor the ones it could not. It prints first now, sharing the filtering that single remove already used to avoid listing a failed delete as a success. The sandbox keeps warning rather than returning. Its lookups are bookkeeping for dependency tracking rather than the user's operation, and the command layer already looks up and reports the same keys, so returning them there as well only prints the failure twice. The same reasoning drops the display lookup from remove when the delete itself failed. Signed-off-by: Justin Chadwell <justin@unikraft.com>
c14bc2c to
e5e3b9f
Compare
jedevc
left a comment
jedevc
left a comment
There was a problem hiding this comment.
@nurof3n can you do a quick pass? I made some modifications, can you check this still works as you intended, but I'd like to merge this now! 🎉
Reviewed-by: Justin Chadwell <justin@unikraft.com>
Approved-by: Justin Chadwell <justin@unikraft.com>
nurof3n
left a comment
nurof3n
left a comment
There was a problem hiding this comment.
changes look good to me!
gotta test for regression as well
To be merged after #388
Closes TOOL-1048