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

add API for crates.io to trigger rebuilds #2534

Merged
merged 8 commits into from
Jul 11, 2024
Merged

Conversation

pflanze
Copy link
Contributor

@pflanze pflanze commented Jun 24, 2024

relates to #2442.

  • adds config variable DOCSRS_TRIGGER_REBUILD_TOKEN /
    Config.trigger_rebuild_token

  • adds build_trigger_rebuild_handler and route
    "/crate/:name/:version/rebuild"

Note: does not yet contain any kind of rate limiting!

@pflanze pflanze requested a review from a team as a code owner June 24, 2024 13:28
@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Jun 24, 2024
Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some first comments.

I'm not 100% sure about the Json/APi error design, but I need a less tired moment to think about it.

src/config.rs Outdated Show resolved Hide resolved
src/web/builds.rs Outdated Show resolved Hide resolved
src/web/builds.rs Outdated Show resolved Hide resolved
src/web/builds.rs Outdated Show resolved Hide resolved
src/web/builds.rs Outdated Show resolved Hide resolved
@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jun 24, 2024
@syphar syphar changed the title Rebuild api add API for crates.io to trigger crate rebuilds Jun 24, 2024
@syphar syphar changed the title add API for crates.io to trigger crate rebuilds add API for crates.io to trigger rebuilds Jun 24, 2024
pflanze added a commit to pflanze/docs.rs that referenced this pull request Jun 25, 2024
As per suggestion on rust-lang#2534, although `id` was ambiguous thus using
`count(*)`.
pflanze added a commit to pflanze/docs.rs that referenced this pull request Jun 25, 2024
As per suggestion on rust-lang#2534, although `id` was ambiguous thus using
`count(*)`.
@pflanze
Copy link
Contributor Author

pflanze commented Jun 25, 2024

I think I've handled all of your suggestions, but I forgot about Clippy. I'll need to get back to that another evening.

@pflanze pflanze requested a review from syphar June 25, 2024 19:42
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jun 25, 2024
@pflanze
Copy link
Contributor Author

pflanze commented Jun 25, 2024

Oh build failures:

error: set DATABASE_URL to use query macros online, or run cargo sqlx prepare to update the query cache

Not sure right now what to do about this (need to stop for tonight).

@syphar
Copy link
Member

syphar commented Jun 26, 2024

Oh build failures:

error: set DATABASE_URL to use query macros online, or run cargo sqlx prepare to update the query cache

Not sure right now what to do about this (need to stop for tonight).

I've put the necessary command into our Justfile, so calling just sqlx-prepare is fine

src/web/builds.rs Show resolved Hide resolved
src/web/crate_details.rs Outdated Show resolved Hide resolved
src/web/error.rs Show resolved Hide resolved
src/web/builds.rs Outdated Show resolved Hide resolved
@syphar
Copy link
Member

syphar commented Jun 26, 2024

Thank you for all the work! This will be awesome for crate authors

@syphar syphar added S-waiting-on-author Status: This PR is incomplete or needs to address review comments and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jun 26, 2024
@pflanze
Copy link
Contributor Author

pflanze commented Jun 27, 2024

I've so far avoided to force-push and instead pushed 3 commits (those with subject lines starting with "sq", standing for squash) with changes that I will squash into the earlier commits where they belong; the idea is that this makes it easier for you to see what I changed; this assumes that diffing is easier as a last step after I do the squashing together with the right commits and force-pushing at the end. Let me know once you're happy and I'll do that. And if that was useful or rather a complication.

@pflanze pflanze requested a review from syphar June 27, 2024 05:33
@github-actions github-actions bot added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jun 27, 2024
pflanze added a commit to pflanze/docs.rs that referenced this pull request Jun 27, 2024
As per suggestion on rust-lang#2534, although `id` was ambiguous thus using
`count(*)`.
@pflanze
Copy link
Contributor Author

pflanze commented Jun 27, 2024

I've realized that you can still see the commit from before me squashing the commits anyway and have now force-pushed db8b363...b7deb0d (i.e. if you'd like to see the incremental changes I did easily, look at db8b363, for the state I wish to be merged b7deb0d).

@pflanze
Copy link
Contributor Author

pflanze commented Jun 30, 2024

I've pushed 3 commits with (a) the 'optimized' SQL query, FWIW, (b)+(c) add and use the constant_time_eq crate for token comparisons. Without it, it would likely be possible for attackers to find the token via response times.

Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really awesome, thank you for the continued work on this.

I'll do some local testing and then merge & deploy this.

( and probably already start using it to queue rebuilds )

@syphar
Copy link
Member

syphar commented Jul 10, 2024

@pflanze do you want to squash some commits before I merge?

- web: `axum_cached_redirect`: change return type to be concrete
- web/routes: add `post_internal`
- test: add `TestFrontent::post`
This aims to satisfy these wishes:

- Use the same error enum (`AxumNope`) for both handlers returning
  HTML or JSON.

- Yet allow those handlers to specify whether HTML or JSON should be
  returned.

- Not change the exising code returning `AxumNope`--still convert to
  HTML by default.

Because `AxumNope` also contains a case (`Search`) that is fixed to
producing HTML, but shouldn't ever be used by handlers returning JSON,
that works with a runtime error in case the latter assumption isn't
being followed.

The approach is to add a new wrapper around `AxumNope` called
`JsonAxumNope` and matching type def `JsonAxumResult`. The wrapper
also implements `IntoResponse`, but produces JSON. Endpoints wishing
to return JSON errors need to use that wrapper and specify
`JsonAxumResult` as their return type.
This resolves rust-lang#2442.

- adds config variable `DOCSRS_TRIGGER_REBUILD_TOKEN` /
  `Config.trigger_rebuild_token`

- adds `build_trigger_rebuild_handler` and route
  "/crate/:name/:version/rebuild"

Note: does not yet contain any kind of rate limiting!
This removes the intermediate `ErrorResponse` type and instead pattern
matches AxumNope on both levels, using a new function
`redirect_with_policy` to avoid duplication.

(Saves 11 lines of code, 7 comment lines, and the indirection via
intermediate type, but adds the function instead.)
@pflanze
Copy link
Contributor Author

pflanze commented Jul 11, 2024

@pflanze do you want to squash some commits before I merge?

@syphar, yes, some more squashing was warranted. I think now it's at the proper resolution.

@syphar syphar merged commit 2eb0516 into rust-lang:master Jul 11, 2024
8 checks passed
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Jul 11, 2024
@pflanze
Copy link
Contributor Author

pflanze commented Jul 11, 2024

This looks really awesome, thank you for the continued work on this.

Thank you, too, for your help during https://rustfest.ch/ and your feedback afterwards! It was the perfect introduction.

Now I just need to get a bit more time.

@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Jul 12, 2024
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.

2 participants