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

Don't try to delete rustdoc files for binary releases #1676

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Feb 13, 2022

This avoids confusing errors like the following:

Error: failed to delete the crate

Caused by:
    <?xml version="1.0" encoding="UTF-8"?>
<Error><Code>MalformedXML</Code><Message>The XML you provided was not well-formed or did not validate against our published schema</Message><RequestId>9F280BB6EBFEC7F3</RequestId><HostId>sgrbwS6rXn83txaNR4V3+YyVm/+VhIrD1Nhaiv9og8H5hNnGq2yntXaGt45DQ8rcmopLRGoPqkg=</HostId></Error>

Tested locally like so:

$ cargo run build crate regex 1.4.1
$ cargo run build crate bat 0.12.1
$ psql postgresql://cratesfyi:password@localhost:15432
cratesfyi=# select path from files where path like '%bat%';
             path
------------------------------
 sources/bat/0.12.1.zip
 sources/bat/0.12.1.zip.index
(2 rows)

cratesfyi=# select path from files where path like '%regex%';
             path
-------------------------------
 rustdoc/regex/1.4.1.zip
 rustdoc/regex/1.4.1.zip.index
 sources/regex/1.4.1.zip
 sources/regex/1.4.1.zip.index
(4 rows)

cratesfyi=# \q
$ cargo run database delete version regex 1.4.1
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
     Running `target/debug/cratesfyi database delete version regex 1.4.1`
$ psql postgresql://cratesfyi:password@localhost:15432
cratesfyi=# select path from files where path like '%regex%';
 path
------
(0 rows)

cratesfyi=# select path from files where path like '%bat%';
             path
------------------------------
 sources/bat/0.12.1.zip
 sources/bat/0.12.1.zip.index
(2 rows)

cratesfyi=# \q
$ cargo run database delete version bat 0.12.1
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/cratesfyi database delete version bat 0.12.1`
$ psql postgresql://cratesfyi:password@localhost:15432
cratesfyi=# select path from files where path like '%bat%';
 path
------
(0 rows)

cratesfyi=# \q

Fixes #899.

r? @syphar

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Feb 13, 2022
This avoids confusing errors like the following:
```
Error: failed to delete the crate

Caused by:
    <?xml version="1.0" encoding="UTF-8"?>
<Error><Code>MalformedXML</Code><Message>The XML you provided was not well-formed or did not validate against our published schema</Message><RequestId>9F280BB6EBFEC7F3</RequestId><HostId>sgrbwS6rXn83txaNR4V3+YyVm/+VhIrD1Nhaiv9og8H5hNnGq2yntXaGt45DQ8rcmopLRGoPqkg=</HostId></Error>
```

Tested locally like so:
```
$ cargo run build crate regex 1.4.1
$ cargo run build crate bat 0.12.1
$ psql postgresql://cratesfyi:password@localhost:15432
cratesfyi=# select path from files where path like '%bat%';
             path
------------------------------
 sources/bat/0.12.1.zip
 sources/bat/0.12.1.zip.index
(2 rows)

cratesfyi=# select path from files where path like '%regex%';
             path
-------------------------------
 rustdoc/regex/1.4.1.zip
 rustdoc/regex/1.4.1.zip.index
 sources/regex/1.4.1.zip
 sources/regex/1.4.1.zip.index
(4 rows)

cratesfyi=# \q
$ cargo run database delete version regex 1.4.1
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
     Running `target/debug/cratesfyi database delete version regex 1.4.1`
$ psql postgresql://cratesfyi:password@localhost:15432
cratesfyi=# select path from files where path like '%regex%';
 path
------
(0 rows)

cratesfyi=# select path from files where path like '%bat%';
             path
------------------------------
 sources/bat/0.12.1.zip
 sources/bat/0.12.1.zip.index
(2 rows)

cratesfyi=# \q
$ cargo run database delete version bat 0.12.1
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/cratesfyi database delete version bat 0.12.1`
$ psql postgresql://cratesfyi:password@localhost:15432
cratesfyi=# select path from files where path like '%bat%';
 path
------
(0 rows)

cratesfyi=# \q
```
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.

Manual test looks good.

Do you mind adding a test for deleting binary releases?

@@ -18,9 +19,15 @@ enum CrateDeletionError {
pub fn delete_crate(ctx: &dyn Context, name: &str) -> Result<()> {
let conn = &mut ctx.pool()?.get()?;
let crate_id = get_id(conn, name)?;
delete_crate_from_database(conn, name, crate_id)?;
let is_library = delete_crate_from_database(conn, name, crate_id)?;
// #899
Copy link
Member

Choose a reason for hiding this comment

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

nit: this comment feels unnecessary here.

@jyn514
Copy link
Member Author

jyn514 commented Feb 16, 2022

Hmm, I'm not sure what an automatic test would look for - just make sure all the proper files are deleted? The change here is "don't try to do something that had no effect", so I'm not sure how we could test that without spinning up a full min.io instance and replaying the logs somehow.

@syphar
Copy link
Member

syphar commented Feb 16, 2022

I was under the impression there was an error happening before this change? ( I admit I didn't check the current state while testing)

We already install minio in GH actions, which is why I assumed that we are already running our tests against it, though looking at the config I'm not sure any more

@syphar
Copy link
Member

syphar commented Feb 16, 2022

Hm... I see previously it didn't fail locally / with minio, so a test for this would be hard.

Approving

@jyn514
Copy link
Member Author

jyn514 commented Feb 16, 2022

I was under the impression there was an error happening before this change?

The error happens when:

  • You try to delete a crate version
  • for a release that's a binary and not a library
  • and uses the S3 backend instead of the database backend

I haven't tested to see if it reproduces with min.io or if it only happens in prod.

@jyn514 jyn514 merged commit c4d041c into rust-lang:master Feb 16, 2022
@jyn514 jyn514 deleted the smarter-version-delete branch February 16, 2022 17:07
@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 Feb 16, 2022
@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 Feb 20, 2022
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.

delete version gives confusing errors if the crate wasn't a library
2 participants