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

database_dump: Fix num_no_build import issue #10097

Closed
wants to merge 1 commit into from

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Nov 28, 2024

The num_no_build column in the database is NOT NULL, but since we don't export it in the database dump it will show up as an empty value and thus fail to get imported correctly. This commit temporarily drops the NOT NULL constraint during the data import, backfills the data from the num column, and then adds the constraint again.

Related:

The `num_no_build` column in the database is `NOT NULL`, but since we don't export it in the database dump it will show up as an empty value and thus fail to get imported correctly. This commit temporarily drops the `NOT NULL` constraint during the data import, backfills the data from the `num` column, and then adds the constraint again.
@Turbo87 Turbo87 added C-bug 🐞 Category: unintended, undesired behavior A-backend ⚙️ labels Nov 28, 2024
@Turbo87
Copy link
Member Author

Turbo87 commented Nov 28, 2024

This seems to fix the issue, but now that I've written the code I'm questioning the brittleness of this approach vs. reverting #9786.

because in downstream code it's trivial to reconstruct from the value of num if needed.

unfortunately, we have 797 versions where it is not trivial, because we have duplications in there. fixing these duplications is possible (see this PR or #9767 (comment)), but 1) does it result in the same data as in the production database? and 2) is it worth the brittleness?

I think I'm leaning towards reverting #9786 instead, but I'm open to other opinions.

/cc @rust-lang/crates-io @dtolnay

Copy link

codecov bot commented Nov 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.47%. Comparing base (a27a914) to head (b71d9d4).
Report is 24 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10097   +/-   ##
=======================================
  Coverage   89.47%   89.47%           
=======================================
  Files         294      294           
  Lines       31254    31254           
=======================================
+ Hits        27964    27965    +1     
+ Misses       3290     3289    -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@LawnGnome
Copy link
Contributor

I think I'm leaning towards reverting #9786 instead, but I'm open to other opinions.

I'd also rather do a revert, I think.

@eth3lbert
Copy link
Contributor

I think I'm leaning towards reverting #9786 instead, but I'm open to other opinions.

Yeah, maybe reverting is a better choice, which would also eliminate the need for potential future maintenance.

@Turbo87
Copy link
Member Author

Turbo87 commented Nov 29, 2024

we talked through this in the crates.io team meeting today and concluded to revert #9786 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-bug 🐞 Category: unintended, undesired behavior
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants