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

Fix overwriting alternate registry token #7708

Merged
merged 6 commits into from Dec 17, 2019

Conversation

@giraffate
Copy link
Contributor

giraffate commented Dec 13, 2019

When executing cargo login, 2nd alternate registry token overwrites
1st alternate registry token.

Fixes #7701.

When executing `cargo login`, 2nd alternate registry token overwrites
1st alternate registry token.

Fixes #7701.
@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Dec 13, 2019

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

Copy link
Contributor

ehuss left a comment

Thanks!

I think it would be best to avoid creating a second registry unconditionally in all tests, since it affects hundreds of tests (creating thousands of extra files). Perhaps some changes like this:

  • Move the code for creating a registry into a separate function (so it is not repeated 3 times).
  • Instead of adding "alt2" functions, add functions with a parameter (like registry_path(name)), and have the existing functions call that instead.
  • Change registry_credentials so that is the only place where the second registry is created. It can modify the .cargo/config file with the [registries] definitions as needed.
@giraffate

This comment has been minimized.

Copy link
Contributor Author

giraffate commented Dec 14, 2019

Thank you for reviewing!

As you say, it seems better to avoid creating a second registry in the other tests, so I fix it. Could you review this again?

I will squashed these commits at the end.

fn generate_dl_url(name: &str) -> String {
let base = Url::from_file_path(generate_path(name)).ok().unwrap();
format!("{}/{{crate}}/{{version}}/{{crate}}-{{version}}.crate", base)
}
Comment on lines 69 to 72

This comment has been minimized.

Copy link
@ehuss

ehuss Dec 16, 2019

Contributor

Can all the other functions (the alt_* functions) be changed to call these new functions instead of duplicating the code?

This comment has been minimized.

Copy link
@giraffate

giraffate Dec 17, 2019

Author Contributor

Yes, I fixed it like that. I also corrected registry_path function etc., but is this OK?

[registries.alternative2]
index = '{alt2}'
Comment on lines 180 to 182

This comment has been minimized.

Copy link
@ehuss

ehuss Dec 16, 2019

Contributor

I think it would be best to not set this here, and only set it in the registry_credentials test. Can probably just append to the config file.

@@ -214,6 +229,23 @@ pub fn init() {
fs::create_dir_all(alt_api_path().join("api/v1/crates")).unwrap();
}

pub fn init_alt2_registry() {

This comment has been minimized.

Copy link
@ehuss

ehuss Dec 16, 2019

Contributor

Can this be written so that the other two registries reuse the same code instead of duplicating it?

This comment has been minimized.

Copy link
@giraffate

giraffate Dec 17, 2019

Author Contributor

I replaced this with a new function init_registry and the existing codes for creating registry call the new function. I am a little worried that there are many arguments for this new function, but I couldn't come up with another good way to write it.

.arg("-Zunstable-options")
.masquerade_as_nightly_cargo()
Comment on lines 195 to 196

This comment has been minimized.

Copy link
@ehuss

ehuss Dec 16, 2019

Contributor

These two lines can be removed, I think we just forgot to remove them when stabilized. And remove the same 2 lines in the call above.

@ehuss

This comment has been minimized.

Copy link
Contributor

ehuss commented Dec 17, 2019

Looks good to me, thanks!

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2019

📌 Commit b7bc069 has been approved by ehuss

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2019

⌛️ Testing commit b7bc069 with merge fc29c9c...

bors added a commit that referenced this pull request Dec 17, 2019
Fix overwriting alternate registry token

When executing `cargo login`, 2nd alternate registry token overwrites
1st alternate registry token.

Fixes #7701.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 17, 2019

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing fc29c9c to master...

@bors bors merged commit b7bc069 into rust-lang:master Dec 17, 2019
10 checks passed
10 checks passed
homu Test successful
Details
rust-lang.cargo (Linux beta) Linux beta succeeded
Details
rust-lang.cargo (Linux nightly) Linux nightly succeeded
Details
rust-lang.cargo (Linux stable) Linux stable succeeded
Details
rust-lang.cargo (Windows x86_64-msvc) Windows x86_64-msvc succeeded
Details
rust-lang.cargo (build_std) build_std succeeded
Details
rust-lang.cargo (docs) docs succeeded
Details
rust-lang.cargo (macOS) macOS succeeded
Details
rust-lang.cargo (resolver) resolver succeeded
Details
rust-lang.cargo (rustfmt) rustfmt succeeded
Details
@giraffate giraffate deleted the giraffate:fix_overwriting_credentials branch Dec 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.