fix(cli): classify failures and stop panicking in the client library#489
fix(cli): classify failures and stop panicking in the client library#489Caesarsage wants to merge 3 commits into
Conversation
|
supersede #481 |
There was a problem hiding this comment.
Please add copyright block at start
| u, err := url.Parse(apiURL) | ||
| if err != nil { | ||
| // url.Parse only fails on a malformed URL; returning it needs a | ||
| // signature change, done with the RunE command migration. | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
This reintroduces a panic in library code, which conflicts with the stated goal of this PR.
Even if malformed URLs are a narrower case, a library constructor still should not panic here. If we cannot change the constructor signature in this PR, then I’d rather keep this work scoped differently than claim we have removed panics from the client layer.
There was a problem hiding this comment.
Thanks, this has been fixed on the next PR
| u, err := url.Parse(realmURL) | ||
| if err != nil { | ||
| // url.Parse only fails on a malformed URL; returning it needs a | ||
| // signature change, done with the RunE command migration. | ||
| panic(err) |
There was a problem hiding this comment.
Same concern here: this keeps a panic path in the client library.
For embeddability, constructors should ideally return errors instead of terminating the host process. If that signature migration has to happen separately, please call that out more explicitly and avoid over-claiming the scope of this PR.
There was a problem hiding this comment.
Thanks, this has been fixed on the next PR
| if enabled, _ := configResp["enabled"].(bool); !enabled { | ||
| return "null", nil | ||
| } |
There was a problem hiding this comment.
We should not silently treat a missing / malformed enabled field as “Keycloak disabled”.
If the response shape is wrong, please return an API error instead. Otherwise a bad server/proxy response can be misclassified as a valid “no Keycloak” setup.
There was a problem hiding this comment.
Similarly, it was addressed on the #490 PR
|
DCO is required. |
c44f139 to
f8c27a0
Compare
Signed-off-by: caesarsage <destinyerhabor6@gmail.com>
Signed-off-by: caesarsage <destinyerhabor6@gmail.com>
Signed-off-by: caesarsage <destinyerhabor6@gmail.com>
f8c27a0 to
732b4ae
Compare
|
fixed @Vaishnav88sk |
|
Suggestion:
|
|
After that LGTM. 👍🏻 |
2 participants
Part of the Microcks CLI v2 work #255.
Introduces a small Failure Kind vocabulary in
pkg/errors(Kind,Wrap/Wrapf,KindOf, and anErrTestFailedsentinel) and makes the client library return classified errors instead of panicking or exiting.Why
pkg/connectorsandkeycloak_clientpreviouslypanic'd on runtime errors (bad response body, unreachable server). A library that can take down its host process is unsafe to embed, which the planned VS Code extension needs to do. Kinds are the library's vocabulary; exit codes stay a CLI concern (added in next PR ()), sopkg/*never depends on them.Notable
Fixes two real latent bugs found while de-panicking:
openIDResp["access_token"].(string): the CLI crashed on a Keycloak 401 instead of reporting it.Scope / safety
Additive and non-breaking: the legacy
CheckError/Fatalshim and the existing constructor signatures are untouched here (removed/changed in later PRs). Build + tests green.