-
Notifications
You must be signed in to change notification settings - Fork 19
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
Adding update version
support
#42
base: master
Are you sure you want to change the base?
Conversation
@bkchr Does it need review/approval or I can hit the merge button? |
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.
Having exclude by passing folders would also be nice.
src/update.rs
Outdated
thread_local! { | ||
/// Packages version - HashMap(name, version)` | ||
pub static PACKAGES_VERSION: RefCell<HashMap<String, String>> = RefCell::new(HashMap::new()); | ||
/// Cargo.lock from URL or FILE sources | ||
pub static CARGO_LOCK: RefCell<Option<String>> = RefCell::new(None); | ||
/// Excluded packages from version updating - HashMap(name, exclude)` | ||
pub static EXCLUDED_PACKAGES: RefCell<HashMap<String, bool>> = RefCell::new(HashMap::new()); | ||
} |
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.
Please no thread locals. Remove them please.
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.
Please no thread locals. Remove them please.
Out of curiosity, is there something wrong about using thread locals? is it a bad practice?
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.
Fixed
fn get_version_by_source(package: &str, source: &VersionSource) -> Result<String> { | ||
let version = match source { | ||
VersionSource::CratesIO => { | ||
let url = format!("https://crates.io/api/v1/crates/{}", package); |
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.
Are you sure that we don't generate too many requests? Maybe we should wait between requests a little bit.
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.
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.
Are you sure that we don't generate too many requests? Maybe we should wait between requests a little bit.
I tested it and it works. Maybe thanks to reqwest::blocking
?
looking forward to this as it simplifies maintenance work for parachain teams. |
This PR aims to solve the problem discussed in this forum post: https://forum.polkadot.network/t/publish-substrate-to-crates-io/949/39?u=nachopal
It adds two new options to the
update
subcommand:--version
: this specifies the source of the versions to which the crates will be updated. There are three valid values:latest
: it will query the latest crate version fromhttps://crates.io/
Cargo.lock
, for example:https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/Cargo.lock
Cargo.lock
--exclude
: this specifies the path to a TOML file where a[diener_exclude]
manifest key is expected, listing the crates we do not want to be updated. (This applies to all kinds of updates, not only for--version
)If it gets approved I can update the
README.md