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

Make .cargo/credentials a subset of .cargo/config #4839

Merged
merged 1 commit into from
Jan 9, 2018

Conversation

sfackler
Copy link
Member

@sfackler sfackler commented Dec 20, 2017

Previously, .cargo/credentials looked like

token = "..."

[my-registry]
token = "..."

And was simply merged into the registry block of .cargo/config. This
meant that custom registry tokens were under
registry.my-registry.token rather than registries.my-registry.token
which is where the index was located, and that you couldn't have a
custom registry named token or it'd conflict with the token for the
default registry.

This commit changes things such that .cargo/credentials has the same
layout as .cargo/config, but only contains token values. For backwards
compatibility, we move token to registry.token when parsing.

@alexcrichton
Copy link
Member

r? @withoutboats

I'd be a little worried if this doesn't break any tests...

@withoutboats
Copy link
Contributor

I believe this PR is correct. My understanding is we have a registry.token field for the default registry, but all fields corresponding to other registries should be under registries.{}, as this PR makes so. I think this was just a discrepency between the PR to make publish work (which uses the token) and the PR to make fetch work (which doesn't).

And the publish tests failed, which shows that the tests were using this behavior. But we should rewrite the tests and fix things to be correct.

@sfackler
Copy link
Member Author

Ok, so things are currently pretty messy. In .cargo/config, we have

[registry]
index = ".."
token = "..."

[registries.my-registry]
index = ".."
token = "..."

while in .cargo/credentials we have

token = "..."

[my-registry]
token = "..."

IIRC registries was used for .cargo/config so that there'd be no conflict with arbitrary registry names - you could have a registry called index or token for example. This problem still exists for .cargo/credentials though. A registry named token will break things. On the implementation side of things, cargo just merges the contents of .cargo/credentials into .cargo/config's [registry] block, which causes this issue with custom registry tokens.

What's the right thing to do here? It seems like the format of .cargo/credentials needs to change to avoid the naming conflict. We also currently load anything from .cargo/credentials, not just tokens which we may want to tighten up?

@withoutboats
Copy link
Contributor

Probably both files should use the same format, so registries in the credentials.

We also currently load anything from .cargo/credentials, not just tokens which we may want to tighten up?

Maybe. It seems confusing but useful to allow anything in credentials - anything you don't want to be backed up can go there (which is probably your credentials, but maybe not).

@sfackler
Copy link
Member Author

Cool, so .cargo/credentials would have the same layout as .cargo/config except that it only contains token fields, right? i.e.

[registry]
token = "..."

[registries.my-registry]
token = "..."

Do you think we need to worry about back compat for existing credentials files?

@withoutboats
Copy link
Contributor

withoutboats commented Dec 20, 2017

Confirm on what credentials should look like.

Do you think we need to worry about back compat for existing credentials files?

If all you're doing is changing the token for alt registries, no because that's an unstable feature.

@withoutboats
Copy link
Contributor

withoutboats commented Dec 20, 2017

Oh, I see. credentials has token at the top level right now!

I think we need to be backwards compatible with that, so we should accept the default registry token at token or registry.token, but all alt registries under the registries table.

@sfackler
Copy link
Member Author

Updated!

@sfackler sfackler changed the title Fix config table name Make .cargo/credentials a subset of .cargo/config Dec 20, 2017
@sfackler sfackler force-pushed the consistent-config branch 3 times, most recently from 2f0012a to 4cc88f3 Compare December 20, 2017 23:49
@sfackler
Copy link
Member Author

sfackler commented Jan 8, 2018

ping @withoutboats

@withoutboats
Copy link
Contributor

Oof, sorry, dropped this after the break.

Could you add a test for the backcompat behavior, just to make certain we don't break anyones' workflow? Otherwise 👍

Previously, .cargo/credentials looked like

```toml
token = "..."

[my-registry]
token = "..."
```

And was simply merged into the `registry` block of .cargo/config. This
meant that custom registry tokens were under
`registry.my-registry.token` rather than `registries.my-registry.token`
which is where the index was located, and that you couldn't have a
custom registry named `token` or it'd conflict with the token for the
default registry.

This commit changes things such that .cargo/credentials has the same
layout as .cargo/config, but only contains token values. For backwards
compatibility, we move `token` to `registry.token` when parsing.
@sfackler
Copy link
Member Author

sfackler commented Jan 8, 2018

Done!

@withoutboats
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 8, 2018

📌 Commit 5cb5e7d has been approved by withoutboats

@bors
Copy link
Contributor

bors commented Jan 9, 2018

⌛ Testing commit 5cb5e7d with merge 4e618dd...

bors added a commit that referenced this pull request Jan 9, 2018
Make .cargo/credentials a subset of .cargo/config

Previously, .cargo/credentials looked like

```toml
token = "..."

[my-registry]
token = "..."
```

And was simply merged into the `registry` block of .cargo/config. This
meant that custom registry tokens were under
`registry.my-registry.token` rather than `registries.my-registry.token`
which is where the index was located, and that you couldn't have a
custom registry named `token` or it'd conflict with the token for the
default registry.

This commit changes things such that .cargo/credentials has the same
layout as .cargo/config, but only contains token values. For backwards
compatibility, we move `token` to `registry.token` when parsing.
@bors
Copy link
Contributor

bors commented Jan 9, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: withoutboats
Pushing 4e618dd to master...

@bors bors merged commit 5cb5e7d into rust-lang:master Jan 9, 2018
@sfackler sfackler deleted the consistent-config branch February 4, 2019 17:21
@ehuss ehuss added this to the 1.25.0 milestone Feb 6, 2022
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.

5 participants