feat: added --dry-run to wkg publish#210
Conversation
| impl Config { | ||
| /// Loads a configuration file from the given path. | ||
| pub async fn load_from_path(path: impl AsRef<Path>) -> Result<Config> { | ||
| tracing::info!(path = %path.as_ref().display(), "loading wkg config file"); |
There was a problem hiding this comment.
It is unfortunate that the tracing deps has to be added for this line but it was particularly valuable to have visibility in what file actually gets resolved here.
mkatychev
changed the title
--dry-run to wkg publishgg--dry-run to wkg publish
| load_package(&mut self.packages, &self.client, dependency.package.clone()) | ||
| .await? | ||
| .await | ||
| .with_context(|| format!("package: {}", dependency.package.clone()))? |
There was a problem hiding this comment.
Looks like a dupe with the next line?
| .with_context(|| format!("package: {}", dependency.package.clone()))? |
There was a problem hiding this comment.
I thought so too, but if you look, the next line is actually an option because load_package returns Result<Option<T>> so the added with_context is if the loading logic failed rather than if the package exists (this issue I specifically ran into).
wasm-pkg-tools/crates/wasm-pkg-core/src/resolver.rs
Lines 601 to 605 in feba4ca
| let name = super::package_ref_to_name(package)?; | ||
|
|
||
| if dry_run { | ||
| return Ok(()); |
There was a problem hiding this comment.
Totally fine if you don't want to make this change but for dry runs I've found it can be nice to model them as an error variant, e.g.
| return Ok(()); | |
| return Err(Error::DryRun); |
There was a problem hiding this comment.
I'm following cargo publish --dry-run logic as much as possible and they seem to throw an exit code of zero.
I also use cargo publish dry runs as CI steps that would abort on exit code so this would cause issues if one wanted to check that a current commit/PR would be able to publish.
There was a problem hiding this comment.
Yeah you would catch the varient down-stack and exit 0. Anyway definitely not a blocker.
lann
left a comment
lann
left a comment
There was a problem hiding this comment.
I'll merge whenever you're ready.
|
Ready now, @lann. I have a follow-up PR to thins that allows |
2 participants
Added ability to dry run publication for the three supported backends attempting to do as many of the present logical steps, stopping short of doing the publication itself.
Added various logging steps for issues I ran into attempting to publish to OCI that were not currently evident.