Fix bin update on prerelease-only github repos#87
Fix bin update on prerelease-only github repos#87sirlatrom wants to merge 2 commits intomarcosnils:masterfrom
Conversation
|
If we're doing this, why not going all the way and letting Thoughts? |
That sounds reasonable, I'll add that in a moment. |
Do we want this though? I've added a comment in #86 |
|
LGTM. Would like to hear @breml's thoughts before merging |
| if err != nil { | ||
| return nil, fmt.Errorf("Error checking updates for %s, %w", b.Path, err) | ||
| } | ||
| if v == "" && u == "" { |
There was a problem hiding this comment.
In which case is it possible, that we do not have an error but still v and u are empty?
While reviewing this code, it was not helpful, that the two variables are named v and u, because this does not tell me anything what I can expect to be in these variables. Also the signature of GetLatestVersion does not tell me more (just that I get 2 strings and an error as return value). Therefore I propose to use some more speaking variable names.
| // Return original 404/StatusNotFound error from GetLatestRelease | ||
| return nil, err | ||
| } | ||
| return release, err |
There was a problem hiding this comment.
I would explicitly return nil for the error to make it obvious for the reader of the code, that this is the happy path.
return release, nil| } | ||
| } | ||
|
|
||
| // Return original 404/StatusNotFound error from GetLatestRelease |
There was a problem hiding this comment.
I think this comment is misleading, because there error could be 404 but it could also be an other error.
| if listErr != nil { | ||
| return nil, listErr | ||
| } | ||
| if len(releases) > 0 { |
There was a problem hiding this comment.
Again, is it possible to not get an error and an empty list of releases? I would expect the API to return an 404 here as well, if there are no releases at all.
|
@sirlatrom by any chance do you have any bandwidth to rebase this? |
Signed-off-by: Sune Keller <sune.keller+gitlab.com@gmail.com>
Signed-off-by: Sune Keller <sune.keller+gitlab.com@gmail.com>
sirlatrom
force-pushed
the
fix-github-prerelease-update
branch
from
0aa8b7e to
e4042ee
Compare
Rebased on "master". |
3 participants
Relates to #86.
Fixes #84.
A future PR could add a (per-binary?) option to offer the latest prerelease if newer than latest release.