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

Transition to TOML as configuration format & some other configuration changes #657

Merged
merged 8 commits into from
Jul 1, 2020

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Jun 18, 2020

Closes #535
Closes #558
Partially unblocks #480

As discussed in #535 the only alternative options were TOML and YAML. There was some discussion without clear result. I applied the "Being Pragmatic" rule from the contribution guidelines and just picked TOML. It has the clear technical advantage of having a significantly smaller parser which improves app loading times. Furthermore, I can't imagine anyone having problems with modifying the example TOML file (given in CONFIGURATION.md) to their liking.

Notes about backwards compatibility: This would in theory be a breaking change. But as we still want to support Opencast 8, we can't break the configuration. As such, JSON is still allowed! To use a JSON config file, REACT_APP_SETTINGS_PATH has to be set and end with .json. This is done in the release scripts for the OC 8 release. For the GET parameter config, JSON can be used, too.
I plan on dropping this deprecated JSON support as soon as we target OC 9.

This is a breaking change!

CC @lkiesow @mtneug @JulianKniephoff

@lkiesow
Copy link
Contributor

lkiesow commented Jun 18, 2020

Please let's not support multiple formats. That just makes everything worse. What about holding this (and maybe all other features) back for a month or two, then bringing this in to target develop in Opencast.

@LukasKalbertodt
Copy link
Member Author

Alternatively, we can also just say that from now on, Studio master targets Opencast develop, and create a legacy branch in this Studio repo for 8.x releases. Let's talk about this Studio/Opencast/Releases/Branches stuff sometime next week, ok?

…tings.toml`

This is a breaking change! The configuration file can still be
overwritten by `REACT_APP_SETTINGS_PATH` which means that if this
variable is defined, this change is backwards compatible. This is
important for Opencast 8.x.

JSON is not a good format for a configuration file. In particular, we
want to add some long strings to the configuration at some point, which
is particularly ugly in JSON.

Viable alternatives are YAML and TOML. While YAML is more familiar to
many people in the Opencast community, TOML was chosen for its
simplicity. This helps humans understand the config file at a glance
and allows for very small parsers. The TOML parser used here is around
8kB minified+gzipped while the alternative YAML parser would be around
40kB minified+gzipped. I doubt that anyone in the Opencast community
would have problems understanding or changing TOML files.
To maintain backwards compatibility, OC 8.x will continue to work with
JSON settings files. But we also want to offer an integrated release
that already works with TOML.
We still support encoded JSON for backwards compatibility. Luckily we
can just check if the decoded string starts with `{`: a stringified JSON
object has to start with that character, while that character is illegal
at the start of a TOML file.
Now only TOML settings are supported.
@LukasKalbertodt
Copy link
Member Author

Ok so, this is a breaking change now: JSON config support was dropped. We can do this breaking change as we target OC 9 now, see #666.

With that, this PR would be ready again. If there are no other problems, I'd merge this the coming days.

…ings, not as a path

Specifying a large XML blob as JSON string was so terrible we decided
to fetch `acl.xml` as another request. But TOML allows us to have good
multiline strings, so specifying the ACL inline gets rid of a bunch of
extra code and makes the app load faster.
@LukasKalbertodt
Copy link
Member Author

I now also changed how ACL is defined: it's inline in the TOML now, not as a path. See last commit for more information. I also rewrite the settings validation, making the whole code a lot more simple.

@LukasKalbertodt LukasKalbertodt changed the title Transition to TOML as configuration format Transition to TOML as configuration format & some other configuration changes Jun 29, 2020
@LukasKalbertodt LukasKalbertodt merged commit 543b0e3 into elan-ev:master Jul 1, 2020
@LukasKalbertodt LukasKalbertodt deleted the toml-config branch June 24, 2021 07:42
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.

Readibility of options table Think about storing settings in a different format (instead of JSON)
2 participants