Make missing plugin repo errors more informative#4087
Conversation
|
I think these fmt.Fprintf(out, "Skipped: %s\n", pr)
fmt.Fprintf(out, "Reason: XYZ (Status Code: %d)\n", statusCode)?
Something to consider for your? |
|
This is quite similar to one of my (forgotten) PRs.
The upstream for all those 3 are all gone/archived :( It's an annoying error but any plugin actions are not affected (Apart from interacting from these 3 plugins ofc). |
Oh, damn. |
|
To be fair, my PR was also doing something else. We can merge this and I can adjust my PR. My PR was also trying to do a reasonable timeout such that a git repo with very slow/unstable connection (notabug was one of them I believe, potentially codeburg as well) won't block the rest of the plugins. |
Now it would look like this (better, thanks!) |
Yes, better. The same applies to: fmt.Fprintln(out, "Failed to decode repository data [", pr, "]:\n", err)Which is quite long too with the causing plugin URL. |
|
Fixed. |
|
@JoeKar that's done, thank you. Anything else? |
Just the fact that we have now 5 commits in the PR history, but we can squash them on merge. |
|
@Neko-Box-Coder is there anything from your side on this? |
Sorry for the late reply, missed this somehow. Nope, seems good to me. Appreciate your work. |
|
@JoeKar is there any obstacle to merge this update? |
| if statusCode != http.StatusOK { | ||
| fmt.Fprintf(out, "Skipped: Unexpected status code %d from plugin repository at %s.\n", statusCode, pr) | ||
| return PluginPackages{} | ||
| } |
There was a problem hiding this comment.
Can we just leverage https://pkg.go.dev/net/http#StatusText instead?
There was a problem hiding this comment.
I just mean that http.StatusText() already provides descriptive strings for all those dozens of status codes (not just these two 403 and 404), so we can just use it, e.g.:
diff --git a/internal/config/plugin_installer.go b/internal/config/plugin_installer.go
index d6f7e2c9..799cf3aa 100644
--- a/internal/config/plugin_installer.go
+++ b/internal/config/plugin_installer.go
@@ -155,23 +155,12 @@ func (pr PluginRepository) Fetch(out io.Writer) PluginPackages {
return PluginPackages{}
}
defer resp.Body.Close()
-
- // Handle cases when plugin repo is not available
- statusCode := resp.StatusCode
- if statusCode == http.StatusForbidden {
- fmt.Fprintf(out, "Skipped: %s\n", pr)
- fmt.Fprintf(out, " Reason: Access to plugin repository is forbidden (Status Code: %d)\n", statusCode)
- return PluginPackages{}
- }
- if statusCode == http.StatusNotFound {
- fmt.Fprintf(out, "Skipped: %s\n", pr)
- fmt.Fprintf(out, " Reason: Plugin repository not found (Status Code: %d)\n", statusCode)
- return PluginPackages{}
- }
- if statusCode != http.StatusOK {
+
+ if resp.StatusCode != http.StatusOK {
fmt.Fprintf(out, "Skipped: %s\n", pr)
- fmt.Fprintf(out, " Reason: Unexpected status (Status Code: %d)\n", statusCode)
- return PluginPackages{}
+ fmt.Fprintf(out, " Reason: Server error %d (%s)\n", resp.StatusCode,
+ http.StatusText(resp.StatusCode))
+ return nil
}
decoder := json5.NewDecoder(resp.Body)
@@ -181,7 +170,7 @@ func (pr PluginRepository) Fetch(out io.Writer) PluginPackages {
fmt.Fprintf(out, "Skipped: %s\n", pr)
fmt.Fprintf(out, " Reason: Failed to decode repository data:\n")
fmt.Fprintf(out, " %s\n", err)
- return PluginPackages{}
+ return nil
}
if len(plugins) > 0 {
return PluginPackages{plugins[0]}| return PluginPackages{} | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
There was a problem hiding this comment.
Unneeded space (in particular, gofmt complains about it).
| statusCode := resp.StatusCode | ||
| if statusCode == http.StatusForbidden { | ||
| fmt.Fprintf(out, "Skipped: Access to plugin repository at %s is forbidden (Status Code: %d).\n", pr, statusCode) | ||
| return PluginPackages{} |
There was a problem hiding this comment.
We return just nil at the end of the function (and also in GetAllPluginPackages()), so we can return nil here as well, for consistency? (Functionality-wise it would be the same.)
There was a problem hiding this comment.
Thank you for explanations!
With your short and simple version we'll get such output:
Skipped: Unexpected status code 404 from plugin repository at https://raw.githubusercontent.com/cadnza/mxc/main/repo.json.
Skipped: Unexpected status code 404 from plugin repository at https://notabug.org/dustdfg/micro-mdtree/raw/master/repo.json.
Skipped: Unexpected status code 404 from plugin repository at https://notabug.org/dustdfg/micro-calc/raw/master/repo.json.
Not every user understands what 404 mean... that's why I handle it separately - to explain "in human terms". From the other side, even knowing user can't do much, just accept. Plugin developers understand what '404' means and what to do about it. I'm ok with such form.
@JoeKar might comment "logs might become quite long" again :)
as my first try looked this way
Skipped: Plugin repository not found at https://raw.githubusercontent.com/cadnza/mxc/main/repo.json (Status Code: 404).
Skipped: Plugin repository not found at https://notabug.org/dustdfg/micro-calc/raw/master/repo.json (Status Code: 404).
Skipped: Plugin repository not found at https://notabug.org/dustdfg/micro-mdtree/raw/master/repo.json (Status Code: 404).
So please you together decide on a perfect form.
There was a problem hiding this comment.
With your short and simple version we'll get such output: [...]
I'm not sure what you are referring to. With my version, the output is:
Skipped: https://raw.githubusercontent.com/cadnza/mxc/main/repo.json
Reason: Server error 404 (Not Found)
Skipped: https://notabug.org/dustdfg/micro-calc/raw/master/repo.json
Reason: Server error 404 (Not Found)
Skipped: https://notabug.org/dustdfg/micro-mdtree/raw/master/repo.json
Reason: Server error 404 (Not Found)
There was a problem hiding this comment.
Could you squash the commits into one? There is no point in polluting the commit history with all these intermediate changes with identical names.
(We could squash it ourselves on merge, but then the commit message would look quite bizarre.)
There was a problem hiding this comment.
Done, looks much better now!
Thank you very much to both of you for your patience, I learned new things!
9604b42 to
1c6b849
Compare
Thank you for this great editor!
Please consider this update to handle missing plugin repo errors better.
It addresses #1276
Plugins in official channel are ok now (may change someday), but 3 plugins of unofficial channel are missing and it shows up as strange messages while running
micro -plugin availableor other plugin commands:Instead it would show up like this: