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

Registry API Command Versioning #6442

Closed
wants to merge 1 commit into from

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Dec 14, 2018

This is a revival of @withoutboats' PR (#4747) with a few other changes. I can split this up if necessary. This includes the following:

  • Add commands to config.json in the index which indicates which version of each command the registry supports. Example:

    {
        "dl": "https://crates.io/api/v1/crates",
        "api": "https://crates.io",
        "commands": {
            "publish":  ["v1"],
            "yank":     ["v1"],
            "search":   ["v1"],
            "owner":    ["v1"],
            "login":    ["v1"]
        }
    }
    • If commands is not specified, it defaults to v1.
  • Change registry commands (publish/yank/owner/login) to work the same as search works – the index is only updated if the config file is missing. None of those commands appear to actually use the index directly and are only interested in the config file. The likelyhood of the config file changing is small. This is probably the riskiest change and worthy of discussion.

  • Rewrite of login command:

    • In order to check the API version, it needs the config file, which means the index will be downloaded if it is not available.
    • The --host flag is deprecated/unused. It wasn't used for much before. Now, the interactive part that asks for the token will use {api}/me as the URL in the interactive message.
    • --registry supports interactive entry of the token (does not require it on the command line).
    • Displays a message after saving the token (fixes cargo login should give feedback on success #5868).
    • Because login now requires an index, some of the tests had to be updated.
  • Fix so that if api is missing from the config, it will display a nice error message instead of panicking with unwrap.

@rust-highfive
Copy link

r? @dwijnand

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

@ehuss
Copy link
Contributor Author

ehuss commented Dec 14, 2018

Note: the Travis issue is due to rust-lang/mdBook#852. I'll look into changing the installed version of mdbook to 0.1.7, which is what we actually use, and doesn't have this problem.

@alexcrichton
Copy link
Member

This all looks and sounds great to me, thanks so much! r=me with CI sorted out

@ehuss
Copy link
Contributor Author

ehuss commented Dec 16, 2018

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Dec 16, 2018

📌 Commit ca49031 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Dec 16, 2018

⌛ Testing commit ca49031 with merge 84442f7dffb001c98b46fb3c281077717ec77607...

@withoutboats
Copy link
Contributor

Thanks for seeing this through! We need to make sure we patch rust-lang/crates.io-index before this lands in nightly or no one will be able to publish from the latest nightly.

@ehuss
Copy link
Contributor Author

ehuss commented Dec 16, 2018

We need to make sure we patch rust-lang/crates.io-index before this lands in nightly or no one will be able to publish from the latest nightly.

I changed it so that it defaults to "v1" if not specified, so no indexes need to be changed. If/when new commands are added, or old ones are changed, then it will need to be updated.

@withoutboats
Copy link
Contributor

@ehuss how would a registry specify that it does not support an operation?

@ehuss
Copy link
Contributor Author

ehuss commented Dec 16, 2018

A few ways:

@withoutboats
Copy link
Contributor

It seems a bit odd for the "default value" to be ["v1"] instead of []. I don't know how many custom registries are out there, but it seems to me like disrupting them now would make more sense than having a surprising default here. Why not?

@ehuss
Copy link
Contributor Author

ehuss commented Dec 16, 2018

@bors r-

It seemed to me the least disruptive choice. This would also impact anyone who has a crates.io development environment. But that's a fair question, and it's just deferring the disruption until we add/change a command. I'll switch it to be required.

@bors
Copy link
Collaborator

bors commented Dec 16, 2018

☀️ Test successful - status-appveyor, status-travis
State: approved= try=False

@ehuss
Copy link
Contributor Author

ehuss commented Dec 16, 2018

Hm, taking a closer look, I'm more concerned about removing the defaults. If someone updates Cargo and runs login, search, yank, owners, or publish --no-verify, it will fail because the index won't get updated with the new values. They'll need to run some other command to update the index first, which is unpleasant and confusing.

This is a bit of a pickle. Ideally Cargo could force an update of the index whenever it is upgraded. However, there's nowhere convenient to track that.

One alternative is to keep these defaults, and figure out what to do in the future when a new API command is added. Perhaps that new command could unconditionally update the index, or do something smarter.

Another idea is to drop this PR, and just rely on the endpoint for versioning. If Cargo wants to use a new version, it could try that (/api/v2/crates/new) and if that 404's, fall back to v1. New commands could have the same behavior (404, report that it can't be done). After thinking about it for a while, this is my preference.

Thoughts?

@alexcrichton
Copy link
Member

Hm that's a good point about updating crates.io's own index, I don't personally feel too strongly here and if others are ok with 404 fallbacks and behaviors that seems reasonable to me!

@ehuss
Copy link
Contributor Author

ehuss commented Dec 20, 2018

Closing in favor of #6466.

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.

cargo login should give feedback on success
6 participants