wasm-pkg-client: validate semver compliance on publish#213
wasm-pkg-client: validate semver compliance on publish#213ryan-surname-p wants to merge 1 commit into
Conversation
| }) | ||
| }) | ||
| .ok_or_else(|| { | ||
| crate::Error::InvalidComponent(anyhow::anyhow!("component package not found")) |
There was a problem hiding this comment.
could we add the id as part of the error message?
|
Hey @ryan-surname-p it looks like some conflicts creeped in -- apologies for the inconvenience but would you mind resolving those? Then we can get this merged. |
|
No problem at all. @mkatychev actually reached out to me about this and some work they have in flight as well. If I understood their request correctly we'd like for that to land first and then work some version of this onto that. This request seemed reasonable enough to me so the current plan is to monitor the progress of 215 and then adjust accordingly. |
|
I'd like to hear @vados-cosmonic's thoughts on which should land first. I suppose one of us will be handling gnarly merge conflicts regardless so it may be fairer for @ryan-surname-p's changes to land first. |
ryan-surname-p
force-pushed
the
oci-client-publish-128
branch
from
cbca048 to
77a1e3e
Compare
|
FWIW I'm happy enough to handle either approach I just wanted to make sure everyone was working with the same set of assumptions. |
| #[tokio::test] | ||
| async fn list_matching_versions_filters_by_version_req_table_driven() { |
There was a problem hiding this comment.
| #[tokio::test] | |
| async fn list_matching_versions_filters_by_version_req_table_driven() { | |
| #[test] | |
| fn list_matching_versions_filters_by_version_req_table_driven() { |
| let cases = [ | ||
| Case { | ||
| name: "target 0.0.0 -> 0.0.*", | ||
| req: "~0.0.*", | ||
| sort: VersionSort::Ascending, | ||
| history: &["0.0.0", "0.0.1", "0.1.0", "1.0.0"], | ||
| expected: &["0.0.0", "0.0.1"], | ||
| }, |
There was a problem hiding this comment.
These tests look very go inspired, pretty nice.
There was a problem hiding this comment.
Now that I think about it, it may be better to reuse rstest for cases and fixtures:
https://github.com/bytecodealliance/wasm-pkg-tools//blob/9dc967c687c6a9ee34c719c5383dfa71ce0cdbc6/crates/wasm-pkg-core/tests/fetch.rs#L13-L21
We could still use the Cases defined and destructure them: https://docs.rs/rstest/latest/rstest/attr.rstest.html#destructuring-inputs
|
|
||
| /// Disable semver compatibility checks. | ||
| #[arg(long)] | ||
| no_verify: bool, |
There was a problem hiding this comment.
skip_semver_check is better than no_verify as an unambiguous flag IMO.
| no_verify: bool, | |
| skip_semver_check: bool, |
| let matching: Vec<VersionInfo> = versions | ||
| .into_iter() | ||
| .filter(|v| predicate.matches(&v.version)) | ||
| .collect(); |
There was a problem hiding this comment.
should we move the filter before the sort_by calls so there is less to sort?
| /// `list_matching_versions` is a generic `VersionReq` filter; this test | ||
| /// exercises it with the cargo-`^` shaped series masks that the publish | ||
| /// gate constructs in `fetch_semver_series` (see `lib.rs`): |
There was a problem hiding this comment.
Is there a reason we have doc comments here?
| { | ||
| tokio::task::spawn_blocking(move || { | ||
| let mut reader = reader; | ||
| let decoded_wasm = | ||
| wit_component::decode_reader(&mut reader).map_err(Error::InvalidComponent)?; | ||
| Ok::<_, Error>((reader, decoded_wasm)) | ||
| }) | ||
| .await | ||
| .map_err(|e| Error::IoError(std::io::Error::other(e)))? | ||
| } |
There was a problem hiding this comment.
Does this function need to produce a future? If we removed the outer spawn_blocking, would we be able to remove the async?
3 participants
closes #128
Summary
By default publish:
The changeset is fairly large I know though a material amount of the new code are tests and moving functions around to keep things readable.