Update max_version on yank #582
Conversation
|
One thing to note, these changes have yet to be tested with the front end. The only thing that comes to mind as a possible problem is the fact that max_version can now be expected to be null. It was never restricted to not being null, but it was never a true possibility due to the backend code. |
|
I have looked into testing the frontend response to a null max_value for a crate. The only test I can think of is to start up a local backend and frontend, publish a crate to the local backend, yank the crate from the local backend, and finally try to view it on the frontend. My problem comes from trying to publish to the local backend using |
Ugh yeah, The next problem that you'll probably run into is that, unless you've set up an s3 bucket, publishing to your local crates.io will fail trying to upload the code to s3. I get around this by temporarily applying this commit to disable uploads to s3 and just update the database-- this has been bothering me as well and I want to fix it as part of #470. Basically I'm sorry i'm sorry. |
|
Alternately, I'm happy to test this out on my setup, since I've already gone through this pain. Let me know!! I'm looking at the code now :) |
|
The code is looking great!! Other than checking out the frontend, the only other thing this change will need is a way to go through existing crates and update those whose max_versions have been yanked to have their next highest non-yanked version used (or set to null if all versions have been yanked) as retep998 reminded me I should have elaborated in the issue, but we have some one-off tasks in src/bin, such as this task that backfilled users' github IDs (ignore the docs at the top of that file, they're copy-pasted from another binary). If you don't want to take care of that for whatever reason, I'm happy to, let me know! |
|
Thanks for pointing me in the right direction! Don't worry about the problems, now we know more about what needs to be improved. I'll be honest, whenever I find a place where documentation is lacking when working with Rust I feel a little better as it reminds me that everyone working on this language/project is human. It's interesting that I'll look at writing the one off task. It should be fairly easy to make as most of the logic is already in |
|
Hmmm, this is a little concerning. I tested publishing a newer version of a crate when the max version was null, and it seems that the max version is not being updated. I'll look at adding a test and code to fix this. |
A crate's max_version is updated when the version we are yanking is the current max_version and when unyanking a version newer than the current max_version.
This is required for the situation where the final non-yanked version is yanked leaving no non-yanked version.
|
Everything you've said sounds great! Also I filed a bug with cargo about the
We are all very much human :) ❤️ |
|
Everything should be wrapped up. I tested the update task against my local copy and it worked. I don't have tests for it, but can add those if you wish. |
|
Just to note, #592 is an alternate solution to this. Even if we do go that route, the tests that this adds are useful and worth keeping. |
|
Oh geez. Before merging either this or #592, I want to hear from alex why max_version was added in the first place. And yes, the tests in here are valuable no matter what ❤️ |
|
I have added tests to the one-off. |
|
@yodaldevoid thank you so much for your work on this, and I hate to do this-- I really should have questioned whether we actually need this column like @sgrif did in #592. I'm definitely going to merge your tests in, which will be valuable in making sure the code in #592 works, but if it does and if production performs ok with the max_version column removed, we might not need this code. But I want to reassure you that your code is great and I'm very grateful!! ❤️ |
|
No problem, really! I'm all for simplifying the database. |
|
Thank you for understanding!!! |
|
Closing because #592 |
Fixes #76