-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Migration: Import compressed archive file handling #8260
Conversation
Will be integrated into bigger PR with UI |
The smaller the PRs are, the easier it is to review them. If that works independently of the UI, I'm in favor a separate PR (I can re-open this one). You probably want to add tests to see it merged though ;) |
Thats an argument. (But it will sit on top of this, I need the zip file handling) |
524125a
to
0cc68c7
Compare
I read this to mean this is WIP or draft. |
No, it's not really a WIP or Draft. This PR is part of the "Migration" Feature. The zipped file support can be used / merged without the other parts. |
So, I'm trying this locally with the rake task and it fails:
|
I see the problem. |
Wow you're actually right, it's because the path is wrong, the file is named I have tons of warning now but it works:
|
My workflow was to drag and drop the file from finder (Mac) directly into the terminal. The Filepath is then copied. |
lib/tasks/accounts.rake
Outdated
@@ -26,6 +26,8 @@ namespace :accounts do | |||
puts "Errors in the archive found:\n#{exception.message}\n-----" | |||
rescue MigrationService::MigrationAlreadyExists | |||
puts "Migration record already exists for the user, can't continue" | |||
ensure | |||
service.remove_intermediate_file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just write ensure
in the MigrationService
in perform!
(above the line of calling remove_intermediate_file
there), that way remove_intermediate_file
doesn't need to be called twice (and it also ensures that the temp files get cleaned up when perform!
is called from somewhere else). It also means remove_intermediate_file
can be private since it doesn't need to be called from external.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SuperTux88 Thanks for reviewing this. In context of #8274 this rake file was more refactored to eliminate duplicate code. So I think for now this file can be as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was more a request to also change MigrationService.perform!
and add the ensure
directly there (and when remove_intermediate_file
doesn't need to be called here anymore it can also be made private).
def perform!
find_or_create_user
import_archive
run_migration
ensure
remove_intermediate_file
end
403b9dc
to
85a5744
Compare
Squashed and merged, thanks 🍪 |
It fixes the '# TODO: archive is likely to be a .json.gz file' in migration_service.rb .
A profile export is downloaded as a gz file, which can not be imported directly before this PR without unzipping.
By uncompressing a gz file, it also handles a zip file (maxOS and Windows create Zip on compressing files by default)
It won't overwrite any existing files.
It removes the uncompressed file after handling it (without removing plain json file, if uncompressed file was given)
@Flaburgan @cmrd-senya What do you think?