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

change latest version ID on crate to match CrateDetails.latest_version #1546

Merged
merged 7 commits into from
Nov 29, 2021

Conversation

syphar
Copy link
Member

@syphar syphar commented Nov 7, 2021

in all release-lists the one version we show should be the latest version (non-yanked and non-prerelease) to match the logic in the rustdoc page header. Otherwise we are showing releases in the lists that directly lead to a page with a go to latest version link.
Now crates.latest_version_id is calculated with the same logic and updated after each build.

This is a preparation for #708, which I would do when #1521 is merged. Then it's just switching the calculated subquery to just using crates.latest_version_id.

The data migration is a very simple impl, and takes 15 minutes on my machine (with macos virtualized docker). IMHO it's fine running that long on prod, of course I can improve speed when needed.

@syphar syphar self-assigned this Nov 7, 2021
@syphar syphar added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Nov 7, 2021
@syphar syphar force-pushed the store-latest-version-on-crate branch from 3e7ec5b to ef71b8a Compare November 7, 2021 14:26
@jyn514
Copy link
Member

jyn514 commented Nov 13, 2021

@Nemo157 would you mind helping review this?

src/web/crate_details.rs Outdated Show resolved Hide resolved
src/db/add_package.rs Outdated Show resolved Hide resolved
Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

LGTM, tested locally and appears to behave correctly.

I assume for the migration we just need to stop building, {update server, run migration} (in parallel/either order), then start building again?

@syphar
Copy link
Member Author

syphar commented Nov 28, 2021

LGTM, tested locally and appears to behave correctly.
awesome, thanks!

I assume for the migration we just need to stop building, {update server, run migration} (in parallel/either order), then start building again?

yep, that would be one way.

Another way would be so move the data migration after the deploy/restart. Since we're only updating data and no schema, this would be fine.

The migration is not really optimized, so it will take some time (locally it were 10 minutes)

I'll fix the conflicts with master, then we can merge.

@syphar syphar added the S-waiting-on-author Status: This PR is incomplete or needs to address review comments label Nov 28, 2021
conn,
&metadata_pkg.name,
&metadata_pkg.version,
&metadata_pkg.version,
Copy link
Member Author

Choose a reason for hiding this comment

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

@jyn514 @jsha without me digging into #1527,
is it correct just to use the version twice?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fine, it only matters for the frontend templates.

src/db/migrate.rs Outdated Show resolved Hide resolved
conn,
&metadata_pkg.name,
&metadata_pkg.version,
&metadata_pkg.version,
Copy link
Member

Choose a reason for hiding this comment

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

Yeah that's fine, it only matters for the frontend templates.

@syphar syphar 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 S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Nov 28, 2021
@syphar
Copy link
Member Author

syphar commented Nov 28, 2021

This seems to be tested and approved now,

I'll merge it when I have time to also deploy it, so this migration is not blocking anyone.

@syphar syphar merged commit 077ee97 into rust-lang:master Nov 29, 2021
@syphar syphar deleted the store-latest-version-on-crate branch November 29, 2021 19:39
@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 Nov 30, 2021
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.

3 participants