-
-
Notifications
You must be signed in to change notification settings - Fork 168
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
Make config migration more robust #670
Conversation
server/src/main/java/dev/slimevr/config/CurrentVRConfigConverter.java
Outdated
Show resolved
Hide resolved
server/src/main/java/dev/slimevr/config/CurrentVRConfigConverter.java
Outdated
Show resolved
Hide resolved
server/src/main/java/dev/slimevr/config/CurrentVRConfigConverter.java
Outdated
Show resolved
Hide resolved
} catch (Exception e) { | ||
LogManager.severe("Error during config migration: " + e); | ||
} |
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.
If anything goes wrong, do we make a backup of the original config? We most definitely should. If it's not viable to do it on an error, then do it before any config migration is done.
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.
Not sure that’s possible with how it is currently set, but Uriel wants to rewrite the whole config system or something anyways…
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.
I think butter is saying to just copy the config file and put it into a .old one
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.
I know, but does the class have access to the file itself? All I see is that it has access to the content… not even the file path, and we can’t access vrServer because it’s in lateinit
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.
sigh, yeah lets leave it for the rewrite
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.
Unfortunate, probably "okay" then as long as it's fairly reliable
Please release 0.7.1 along with this