Skip to content
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

Teach publish the --dry-run flag #1699

Closed
wants to merge 2 commits into from

Conversation

JustAPerson
Copy link
Contributor

Closes #1332

Open to feedback on rewording messages and such. I tried to follow this precedent for erroring on conflicting flags.

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

try!(config.shell().status("Uploading", pkg.package_id().to_string()));
try!(transmit(&pkg, &tarball, &mut registry));
if !dry {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this check be pushed into the transmit function actually? There's a few more errors that can happen right before the publish and it'd be nice to catch those.

@alexcrichton
Copy link
Member

Nice! Could you also add a test for this option?

Catch a few more errors by aborting midway through transmit().
@JustAPerson
Copy link
Contributor Author

@alexcrichton,

I've moved the check into transmit as requested. Additionally, I've written three tests that check the stderr/stdout of the process, but I'm not sure I can easily figure out how to check the local crates api files to ensure that the packaged crate was never uploaded properly. There's an example of checking the local api but I'm not sure what my tests should actually be looking for here. Is this step necessary?

Ignoring the issue of checking whether or not the upload actually occurred, the tests I've written cover:

  • error on conflicting flags --no-verify and --dry-run
  • warn "aborting upload due to dry run" after normal output
  • error when invalid readme path specified in Cargo.toml, which is checked in transmit()

@alexcrichton
Copy link
Member

Those tests sound good to me! You maybe able to configure a host that's something like http://example.com which will never have a successful publish perhaps?

@alexcrichton
Copy link
Member

It seems that this has stalled, so I'm going to close this, but feel free to reopen with a test added!

@erickt
Copy link
Contributor

erickt commented May 3, 2016

@JustAPerson: Any chance you could revive this? I just was thinking how wonderful this feature would be to verify a package is actually publishable before it's tagged with the release version.

@JustAPerson
Copy link
Contributor Author

@erickt,
Sorry about that, not really sure why I gave up on this previously.
I should finally have some time this weekend after a busy semester.
I'll try to write up a few simple tests so this can be merged.

@erickt
Copy link
Contributor

erickt commented May 6, 2016

@JustAPerson: No worries, I know what that's like. @alexcrichton keeps closing my rust PRs because I haven't had time to get them over the finish line yet :)

bors added a commit that referenced this pull request Jul 18, 2016
Add --dry-run to cargo publish

This PR picks up where @JustAPerson left off in #1699. I've updated their changes to apply to current master and added tests, including (I think) a test that checks that the upload doesn't actually occur.

The output is as it was before:

![output](https://cloud.githubusercontent.com/assets/21787/16720362/f7ea710e-4778-11e6-8163-65f3978bb7ae.png)

Closes #1332
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants