Bail publish job before packaging and upload (passing tests) #14338
Conversation
|
r? @weihanglo rustbot has assigned @weihanglo. Use |
rustbot
added
A-interacts-with-crates.io
torhovland
force-pushed
the
same-version
branch
from
ef215c7
to
a6aca5f
Compare
|
@hi-rustin, you recently refactored away the return tuple in |
The goal with that refactor is less about removing the tuple and more about pulling work out of that function for reuse, like @jneem's recent |
src/cargo/ops/registry/publish.rs
Outdated
|
|
||
| if !duplicate_query.is_empty() { | ||
| bail!( | ||
| "crate {} already has version {}. Aborting publish.", |
There was a problem hiding this comment.
nit: what do you think of
"cannot publish {}@{} as that version already exists on {registry}",or
"a {}@{} already exists on {registry}",There was a problem hiding this comment.
Updated by @stupendoussuperpowers.
tests/testsuite/publish.rs
Outdated
| .add_responder("/index/3/f/foo", move |req, server| { | ||
| let mut lock = arc.lock().unwrap(); | ||
| *lock += 1; | ||
| if *lock <= 1 { | ||
| server.not_found(req) | ||
| } else { | ||
| server.index(req) | ||
| } | ||
| }) | ||
| .build(); |
There was a problem hiding this comment.
I can't remember, was there a reason we couldn't publish the package like we normally do in tests?
There was a problem hiding this comment.
This was initially added because publish during tests did not have any memory of previously uploaded packages by default. Not sure if that changed, but test-publish works fine without this responder now.
There was a problem hiding this comment.
And it's updated.
|
As for the 2 failing tests - credential_process::token_caching - This is failing because the test case tries to upload a package twice to test out two different scenarios. However it does it with the same version number and therefore our new code changes mean that the second test publish job bails. I have verified a fix for this locally where we just need to change the version and expected stdout. registry_auth::token_not_logged - Here, we are logging the token 8 times instead of the previous 7. I wonder if this is because of the extra check we make to confirm whether the crate@version exists already? If so, we can either change the test to expect 8 logs (if that is the new accepted behaviour), or check why there's an extra log. Let me know how I should proceed if we are okay with continuing with this solution. CC: @torhovland |
|
I think this approach, rather than |
|
@torhovland - all tests are passing with this. you can apply this patch and we should be good to go. |
torhovland
force-pushed
the
same-version
branch
from
a6aca5f
to
b5be434
Compare
|
Good job! @epage This is ready for another look. @stupendoussuperpowers Will you follow up any feedback, please? |
|
I'm assuming I'll have to rebase to be 2 commits - first with the test and second with the updates. Will get to it. |
|
For simplicity, feel free to reopen #13501 and finish the work there. |
I think you are attempting to return a different type. What I removed from it is |
| // 5. /index/3/f/foo for checking duplicate version | ||
| // 6. /api/v1/crates/new | ||
| // 7. config.json for the "wait for publish" | ||
| // 8. /index/3/f/foo for the "wait for publish" | ||
| assert_eq!(authorizations.len(), 8); |
There was a problem hiding this comment.
Can this be squashed into the appropriate commit? Ideally all commits pass tests
tests/testsuite/publish.rs
Outdated
|
|
||
| p.cargo("publish") | ||
| .replace_crates_io(registry_dupl.index_url()) | ||
| .with_stderr( |
There was a problem hiding this comment.
Can this commit be updated to be using the current testing style, rather than deferring that to later commits?
| } | ||
| }) | ||
| .build(); | ||
| let registry_dupl = RegistryBuilder::new().http_api().http_index().build(); |
There was a problem hiding this comment.
Can we update the original test with these changes?
|
It'd be a big help for the commits to be cleaned up Either
or
|
|
☔ The latest upstream changes (presumably #14433) made this pull request unmergeable. Please resolve the merge conflicts. |
|
#14448 was merged with these fixes. I believe we can close this. |
Builds on #13501 with a fix that makes the tests pass. Fixes #3662.