-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: get versions from the repo directly #11
Conversation
README.md
Outdated
``` | ||
|
||
### Offline | ||
|
||
You can use PSVM offline with the `-v` or `--version` flag, cause PSVM keeps a local mapping (i.e. [v1.3.0 Mapping](/src/versions/release-crates-io-v1.3.0.json)) of the Polkadot SDK dependencies versions, that will be slowly updated. So be aware that the latest Polkadot SDK versions may not be available in the local mapping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure that we need the offline version at all? As soon as there are two ways to do something and one is wrong, then ppl will end up doing the wrong thing and complain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I keep it to just not cause a breaking change with the "offline" justification, but probably is useless cause anyways you need internet to download the crates after setting a version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only concern here is the fact that some branches don't have a Plan.toml file for example. What do you about fallback to the Cargo.lock in case Plan.toml not found. For example v1.3.0: https://raw.githubusercontent.com/paritytech/polkadot-sdk/release-crates-io-v1.3.0/Plan.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the Cargo.lock fallback is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! It's done, also added a test to check if it works for all release branches, and it works! The only branch without Plan.toml is v1.3.0, but in this case it will fallback to Cargo.lock. Added a warning in the Workflow section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Hey @patriciobcs, thanks for this change! I wonder, what is the reason to use I'm asking because I'm having some issues with some of the crates when using In my case, |
The Cargo.lock file may contain dependencies that are not published in crates.io, and the tool will not be able to filter them out cause it is not possible to determine if a crate is published or not (with this file). If you have a local dependency with a name similar to a crate not published, the tool will overwrite it, so be careful. Adding an extra argument to use directly the Cargo.lock can also be done, but be aware of this issue. |
Thanks @patriciobcs I see what you mean.
That is true. However, with The Plan.toml file may contain dependencies that are published in crates.io, but marked as How so? The Example:
I can think of three solutions to this:
|
Solves: #9
This changes the tool workflow, reducing to almost none its maintenance. Now, it will always fetch the versions directly from the repo using raw files. It seems the rate limit is 5000 queries per hour (ref). Considering the tool will max fetch 2 times (when falling back to Cargo.lock). The rate-limit is totally acceptable. If anyone would want to have a greater limit, in the future this can be replace by direct API usage by using GitHub CLI or API tokens.