-
Notifications
You must be signed in to change notification settings - Fork 10
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
Allow user to import KA Lite database during upgrade. #462
Allow user to import KA Lite database during upgrade. #462
Conversation
@mrpau-richard sorry that there have been no reviews here Could you explain why importing a database is only relevant during an upgrade? |
@benjaoming I think the reason why I have this PR is when we allow the user to set their own KA Lite content path, it also may need to import their existing KA Lite database. I suggested it in this PR. |
@benjaoming it's also applicable to import database during fresh installation. Sorry I did not get your question here. |
@mrpau-richard would you say that this is safe to merge? If an upgrade through the Windows Installer causes the database to be overwritten presently, then it would sound like a critical fix. However, if the current behavior also keeps an existing database unharmed, then maybe we should skip this PR, seeing that we probably won't be able to maintain any possible bugs that it might cause (not saying that the code is bad, just that it's rather complex by nature) ? |
@benjaoming I just re-tested this, During the upgrade it will overwrite the existing KA Lite database and replace with the new one that the user uploaded. |
@mrpau-richard wasn't the old behavior during upgrade to keep the user's existing database? I mean, the same as what would happen if you |
@benjaoming Yeah, During upgrade the old KA Lite database remain. Unless the user uploaded a new one and it replace the existing database. |
Thanks for the clarification @mrpau-richard -- then I would say that this issue is not important: Replacing the database while upgrading does not fit any use case that I know of. Preserving the database is the critical part, and if that's already covered, then I think it's okay to move on. Would you be okay about closing this PR? We have a lot of stuff going on :) |
I agree @benjaoming let's close this and move on. |
Summary
Issues addressed
Fixes #459
Here's the windows installer for testing.
/cc @radinamatic @benjaoming