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

drop DB before importing #1389

Closed
wants to merge 3 commits into from
Closed

drop DB before importing #1389

wants to merge 3 commits into from

Conversation

obfusk
Copy link
Contributor

@obfusk obfusk commented Jun 23, 2023

Split off from #1343:

  • drop DB before importing
  • warn user about data being dropped
  • delete old images

@obfusk obfusk marked this pull request as ready for review June 23, 2023 22:04
@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

This drops the DB. But I forgot about the card images. We should probably also delete those.
But that's more complicated since we can't use the DB transaction for that and file names may be reused.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

Looks like the unit test is stuck again :/

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

I forgot about the card images.

So as a result imported cards will sometimes show old images. Seems to work well otherwise in my tests.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

But that's more complicated since we can't use the DB transaction for that and file names may be reused.

So we'd probably have to make a list of existing files before the import. Then subtract the files after the import and delete the rest.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

Will look into that. But not today.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

I don't think the images are ever deleted currently, even when the card is deleted.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 23, 2023

I don't think the images are ever deleted currently, even when the card is deleted.

They are. I just looked in the wrong place.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 24, 2023

I added code to remove the old images.

@obfusk
Copy link
Contributor Author

obfusk commented Jun 24, 2023

This won't delete any existing image files unless the import succeeds. But existing image files may be overwritten by a failed import.

To avoid that we'd have to e.g. save them to a temporary directory instead and move them after the import succeeds. I can make a follow-up PR for that if you'd like, but this seems like a worthwhile improvement already and I don't want to make it unnecessarily complex.

@obfusk obfusk marked this pull request as draft June 26, 2023 14:15
@obfusk obfusk mentioned this pull request Jun 26, 2023
@obfusk obfusk closed this Jul 16, 2023
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.

1 participant