feat: switch to using /releases/latest api for getting stable version#360
feat: switch to using /releases/latest api for getting stable version#360hmnd wants to merge 3 commits into
/releases/latest api for getting stable version#360Conversation
/releases/latest api for getting stable version
MordechaiHadad
left a comment
There was a problem hiding this comment.
This a much better solution than fetching all the neovim releases.
|
The checks are failing cuz of a PR I just merged, which changed the deserialization implementation. |
|
Technically, this does not get the stable release, it gets the release marked as latest. There is an API to get the actual release tagged as stable: https://api.github.com/repos/neovim/neovim/releases/tags/stable Practically, latest would probably always be the latest stable. |
|
From what you're saying @aeons (and the code changes), this no longer fetches stable releases correct? I'm in the process of writing #348 - which means larger scale changes - either those that prefer calling the API or changes to implementation details and version flows would be somewhat overkill, though I say this understanding this is (currently) preventing users from pulling certain releases down. Is there a way to get this up & running without too many deep/large changes? |
|
@aeons the reason I went after latest and not stable is that the code uses the tag name of the fetched release as the version number, and the stable release is tagged But, I've come up with a more elegant and reliable way to do this. Now, we fetch the |
| /// let stable_version = get_upstream_stable(&Client::new()).await?; | ||
| /// println!("Stable version: {}", stable_version); | ||
| /// ``` | ||
| pub async fn get_upstream_stable(client: &Client) -> Result<String> { |
There was a problem hiding this comment.
I'm not really sure why the need to over-complicate this compared to the previous of just fetching latest? In bob stable and latest for neovim version is rather synonymous, could there be an instance where latest isn't actually the latest version? Possibly, but I don't really remember such an instance.
What you can do is take target_commitish and compare to the commit hash to the first one inside https://api.github.com/repos/neovim/neovim/tags since they seem to be ordered from the latest tags to the oldest tags.
Which if this fails, run your additional tags/stable request with lets say 50? (we can probably even store the one from the first check anyway) and if this one works push.
So it will look something like api latest fetch > first tag fetch > compare > api tags stable fetch > compare to first tag that was fetched.
There was a problem hiding this comment.
Actually scratch that, I think I have a better idea 1 moment.
There was a problem hiding this comment.
Okay never mind let's just do this, there are 2 solutions:
- keep the current implementation, set the ?per_page to around 10 (even 10 i think is overkill considering the last semver version will always be on top, if stable and tags comparison fails just resort to release/latest
- just do release/latest
I think both will rarely fail as the first implementation seems to always sort the latest tag the first but it introduces 3 different requests could be even more if we implement pagination like nvchecker which is used by arch linux, or have 1 maybe not the most accurate if someone makes a mistake in setting the correct latest, but it will result in only 1 request.
There was a problem hiding this comment.
Quick note regarding things like pagination and what not - the implementation that I'm working on will effectively handle interactions like this "upfront" for sections that need to know anything about version values etc.
I'll push up the local fiddling I've been doing (to just my own repo) so you can get an idea of the interface.
There was a problem hiding this comment.
@hmnd waiting on you to roll back so we can merge this asap.
There was a problem hiding this comment.
Important note here with:
introduces 3 different requests
This isn't too big of an issue with tokio/async Rust as we can path them viatokio::join!( { } )which you can think of from JS world as somewhat likePromise.all()- assuming we can give each of the req's a copy of a client.
Could we simply expose this number to the users/config in the meantime as a temp. fix? Allowing users to update it (5/10/15 etc.), moves the immediate need to tweak it all the time to the user (not good, but... is a work-around).
|
I'm fine with using the latest release. It's the simplest solution and will probably(tm) be the same as the stable release. I just wanted to highlight that it is not technically, semantically the same. Maybe just changing the wording would be enough. |
|
@hmnd waiting on you to finish the appropriate changes so we can merge this hotfix |
|
Closing due to merging a co-authored PR. |
|
Apologies for not responding sooner - dealing with some things in my personal life. Thank you for picking it up and getting it merged! |
Ah, I see hope it's all right now. |
#fixes #356
As described in my comment #356 (comment)