Simplify builds.json to return what the clients actually want #2144
Conversation
github-actions
bot
added
the
S-waiting-on-review
AFAIK our primary consumers are: * [shields.io](https://github.com/badges/shields/blob/bf1bea8b4a106227cd81a9c5ac88e0266cf9c2c2/services/docsrs/docsrs.service.js#L4-L11) * [crates.io](https://github.com/rust-lang/crates.io/blob/609a7f312429a0e3c99714858b7e7655c8fe36e4/app/models/version.js#L136-L143) They both only care: > will going to `https://docs.rs/<crate>/<version>` show docs They emulate this by checking whether the latest build is successful, but this relies on docs.rs also using this as the signal (it currently mostly is for other reasons, but we _should_ show docs in cases where a previous build was successful but a newer rebuild failed, and there were previously bugs around proc-macros). So, remove all the extraneous data they don't depend on and just return a simple boolean showing whether the docs are good or not, the same one we use to determine whether to redirect to show a build failure warning or not: https://github.com/rust-lang/docs.rs/blob/f1a7e46a620d763db485a25efb5e510fd3fe0594/src/web/rustdoc.rs#L476
There was a problem hiding this comment.
So, in general I like the idea of simplifying things :)
The general idea sounds good to me, though I didn't check the shields.io/crates.io implementation details.
Just a thought:
We are removing the list of builds here, and adding a new attribute. That means even when both crates.io and shields.io migrate , there will be a migration phase where we have to keep the old contents live and already return the new attribute, so services can migrate.
Wouldn't it then be easier to return the new content in a new route (like build_status.json), separately tracked?
Then we can easily see if we have other users accessing the old endpoint, after crates.io & shields.io migrated.
|
We're retuning the same structure they expect. I was considering adding a new route with a different structure, but at least at the moment I don't think it's worth the migration pain. |
You're right, I missed the |
if it's worth the migration pain probably also depends on your initial question, if anyone except crates.io / shields.io uses this endpoint / the result. I see two ways here:
From my limited experience I couldn't say if we can just do the breaking change, without the data above. |
|
(it's not obvious in the rendered markdown, but the crates.io/shields.io links above are directly to the code they use to parse it) |
|
A thing I just remembered : since the endpoint is uncached, you could also check the access logs on the server itself, if they contain what we seek |
|
I did that, we just get |
|
I just thought of another major consumer: lib.rs, that appears to deserialize the other fields but doesn't read from them, I'm not sure if that will fail to deserialize if they're removed (cc @kornelski). |
|
I can update lib.rs for the new format, no problem. |
|
I've been persuaded to go the new endpoint route, I'll open a separate PR adding that and prepare PRs for crates.io/shields.io (and lib.rs assuming my gitlab login still works) using it for once it's been deployed. Then we can re-evaluate whether there's still any traffic to this endpoint. |
AFAIK our primary consumers are:
They both only care:
They emulate this by checking whether the latest build is successful, but this relies on docs.rs also using this as the signal (it currently mostly is for other reasons, but we should show docs in cases where a previous build was successful but a newer rebuild failed, and there were previously bugs around proc-macros).
So, remove all the extraneous data they don't depend on and just return a simple boolean showing whether the docs are good or not, the same one we use to determine whether to redirect to show a build failure warning or not:
docs.rs/src/web/rustdoc.rs
Line 476 in f1a7e46
(This helps simplify some changes I'm working on around what and how we store build details).