-
Notifications
You must be signed in to change notification settings - Fork 133
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
Server settings redesign #266
Comments
Is it really necessary to change the file format for the configurations?? |
I have shown the benefits above. We can use the same file's base name (as in, only change the extension) and fall back to the old settings (if no "config.json" is found) until the following major version, in which we'd remove the old settings model. Also note that it's not much a big deal that we're changing the format, considering that we'll have to change the whole implementation anyway.
JSON is less complex than XML, it is relatively easy to use, and doesn't have as many caveats as those found in XML documents (we don't need text parts between elements, for instance). Not to mention that if our users are manipulating XML by hand, then something is not right here either. Don't we already have web services and UI components in our web app for this?
Migrating the schema to a similar one in XSD is possible. If we really end up doing so, will you help me do the translation? |
Here's a branch with a nearly full implementation. The tests for loading configuration files are passing for both definitions. |
I have changed the same branch (here) to map configurations to XML instead of JSON. |
@tmgodinho Feedback? |
nice! now, as discussed, we need to make sure that we make the migration for the legacy file to this new versions. And as discussed previously, let's try to move them to ./confs directory. There are several advantages of having it in a separated folder, for instance, in docker, we can keep configurations outside. |
After some tweaks in the legacy code, I've successfully added migration to the branch, here's the passing test. 🎉 The next 3 steps are:
|
Resolved by #284. |
Many of our future issues (e.g. #98 #122 and #255) will require more core server configurations. Given the current, difficult state of our server settings design and implementation, this issue is a proposal for a redesign and reimplementation of dicoogle's server settings (what is currently known as config.xml).
The new format is still based on XML, but with a different, more manageable schema.
You might find that some fields are missing. They were either deemed redundant or just made of obsolete settings that no longer belong in the core settings.
I could be missing some important fields though, so feedback is important.
We are also refactoring the programmatic API (see the new
ServerSettings
andServerSettingsReader
). We'll be using a setter and a getter for each property, with methods for reaching each configuration namespace. Currently, there are 3 namespaces, one of them with 2 namespaces: "web-server", "archive", and "dicom-services" (containing "storage" and "query-retrieve").You may find a configuration example here. It's currently being used for testing too. :)
Proof of concept: https://github.com/bioinformatics-ua/dicoogle/tree/imp/server-settings-refactor
The text was updated successfully, but these errors were encountered: