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鈥檚 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
3d464ff to
5c8e9aa
Compare
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>
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.
| if err != nil { | ||
| errs = append(errs, err) | ||
| } |
There was a problem hiding this comment.
Ah, yes, because errors.Join ignores nil errors, we can simplify this:
| errs = append(errs, err) |
Keeps this block nice and small.
| if err != nil { | ||
| log.G(ctx).Warn().Err(err).Msg("instance created with partial lookup errors") | ||
| } |
There was a problem hiding this comment.
How can this happen? I'm not sure I'd know what to do if I was greeted with this error.
To be merged after #388
Closes TOOL-1048