Skip to content

transfer_crates script: switch to GitHub IDs #858

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

Merged
merged 4 commits into from
Jul 15, 2017
Merged

transfer_crates script: switch to GitHub IDs #858

merged 4 commits into from
Jul 15, 2017

Conversation

vignesh-sankaran
Copy link
Contributor

Redo of #798, learned not to use master for PR's :)

@vignesh-sankaran vignesh-sankaran changed the title Transfer crates: switch to GitHub IDs transfer_crates script: switch to GitHub IDs Jul 8, 2017
@sgrif
Copy link
Contributor

sgrif commented Jul 8, 2017

@vignesh-sankaran Can you rebase this please? Something funky happened with your branch setup. Shouldn't need 10 commits for a 7 line change

@carols10cents
Copy link
Member

Hi @vignesh-sankaran, sorry it's taken me so long to review this!

Yes, as sgrif mentioned, please rebase on rust-lang/crates.io master and force push to this branch to get rid of all the merge commits-- please let me know if you have questions about how to do that!

Regarding some of the comments you had on the last PR:

A couple of things I noticed were that the script exited if an uppercase "Y" was entered

That's fine

and that there is an assertion that fails if more than 1 crate is being transferred.

I think that's because when the crate_owners table used to have an id column, and this script used the crate_owners id instead of the owner_id, then this script would update one record each time through the for loop. I think now that this table doesn't have an id, and we're using the owner_id and updating all the crates at once, the assertion should be removed. In fact, the UPDATE query can now be moved out of the for loop and executed once after the loop. Could you make those changes please?

As a matter of interest, when is this script utilised?

I believe this is for when someone has created a new github account and wants to transfer all their crates from the old account to the new account. I don't think this happens very often anymore (I don't think I've ever needed to run this script), but there's still a possibility of needing to do this. And actually, now that I think about it, because we're now using github ids and we have a uniqueness constraint on github id, the warning in this script will always be printed.... I think it's fine though :)

Thank you! 🌷 🌻

@vignesh-sankaran
Copy link
Contributor Author

@carols10cents I've implemented the changes as requested :)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Moved update statement outside for loop and removed singular crate owner transfer assert.
}

let _ = tx.execute(
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need the let _ = at all here anymore; the .unwrap() should make sure the result is getting used :) Could you remove let _ = here please? Thank you!!

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Removed `let _ =` on database transaction
@carols10cents carols10cents merged commit 66d5d92 into rust-lang:master Jul 15, 2017
@carols10cents
Copy link
Member

Thank you!!! 🍰 💃 🌊

@vignesh-sankaran vignesh-sankaran deleted the transfer_crates branch July 16, 2017 11:15
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.

None yet

4 participants