Closed crudiedo closed 1 year ago
The plan looks great to me! 👍🏻
The only problem, without the GitHub token, there's a quite limited number of requests a tool can do. If we add an extra request to fetch tools, it makes tool-sync
less usable for such users.
So it makes sense to minimise the total number of requests as much as possible. I agree that it can be unavoidable sometimes to provide better UX but still.
My alternative proposal would be to submit a separate request to GitHub only if the releases/latest
is not found. Every GitHub repo has the latest release (if it has at least one). Could we check if GitHub returns different results for:
owner/repo
owner/repo
without an releases?
@chshersh thanks, it makes sense.
Regarding the different results - Github API always returns 404 with
{
"message": "Not Found",
"documentation_url": "https://docs.github.com/rest/reference/repos#get-the-latest-release"
}
no matter if you mistyped the repo or the tag.
I've also checked that on the repo with no releases and the latest tag, and got exactly the same error, so we won't be able to identify the reason based on the client.fetch_release_info()
response only :(
I believe it would be a rare case when somebody wants to fetch a repo that has no releases at all, so we might want to avoid that check\separate request if the releases/latest
tag is not found since, as you mentioned, it should exist for every repo.
I propose to do it this way then:
If client.fetch_release_info()
returns 404 (the reason could be either repo or tag is not found)
latest
, show the The Repo is not found, please make sure the configuration is correct
errorEither repo or release is not available, please make sure the configuration is correct
errorThis way, we wouldn't make any additional requests, and also partially cover this https://github.com/chshersh/tool-sync/issues/69 issue until we figure out something better.
@crudiedo This sounds excellent to me 💯 And thanks for checking how the API works 👍🏻
My only suggestion is for the error message when the tag is latest
: let's also mention that the repository might not have any releases:
-The Repo is not available, please make sure the owner and repo are correct
+The {owner}/{repo} doesn't exist or has no releases.
In theory, we could output a better error message when we see a GITHUB_TOKEN
env variable. But it'll complicate the code significantly so let's just output messages that tell about potential multiple errors.
Currently, if we mistype owner/repo in the conf file, we get this error:
It looks a bit confusing for the user since it looks like something tags\releases related.
I propose to:
RepoError::NotFound(owner, repo)
error and implementfmt::Display
for it (something like "Repo {owner}/{repo} is not found, please check if it's available")head
request tohttps://api.github.com/repos/{owner}/{repo}
before callingfetch_release_info
ureq::Error::Status(404, _)
on the head request above and showRepoError
insteadI believe this would also work for the repos that got deleted.