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

Fail fast on misconfigurations #1188

Open
lkiesow opened this issue Sep 12, 2024 · 1 comment · May be fixed by #1193
Open

Fail fast on misconfigurations #1188

lkiesow opened this issue Sep 12, 2024 · 1 comment · May be fixed by #1193

Comments

@lkiesow
Copy link
Contributor

lkiesow commented Sep 12, 2024

I've just spent a day of debugging since the configuration format of Studio changed between Opencast 14 and Opencast 16. While it wouldn't have helped in this specific case (see #1187), it would be great if Studio would publically state the misconfiguration (alert, error on page), instead of silently ignoring/changing the configuration in the background.

If the configuration is invalid, the admin probably tried to change something and that clearly didn't work. Therefor, he needs to know that as soon as possible to intervene and fix the configuration.

Screenshot from 2024-09-12 14-46-21

Just printing a warning in the development console is insufficient, since that's not something people will usually see.

@dennis531 dennis531 linked a pull request Oct 14, 2024 that will close this issue
@LukasKalbertodt
Copy link
Member

Sorry for the delay replying to this.

I'm not 100% convinced, or rather: changing this does have at least one disadvantage as well. If Studio hard rejects unknown config values, we somewhat lose forward compatibility. Consider an LMS plugin adding a new feature that uses a new Studio configuration. If older Studio versions (without that new config) would fully fail, then the LMS plugin suddenly has Studio version requirements: it requires a minimum Studio version to work. So the LMS authors might be hesitant to push that new feature, since it would cause work for admins.

I know a few serialization libraries where the default is to "ignore unknown fields" while deserializing, for exactly this reason. I think that's a sane default. And I think a very similar argument can be made about the "value check", e.g. if a newer Studio version will allow additional values for a configuration.

If this forward-compatibility wasn't given, LMS plugin authors would either check the Studio version (which I don't think is simple) or have a configuration where an admin can specify the Studio version. Not terrible, but not great either.

I of course also understand where you're coming from. Quickly see the error is a good thing. With the argument above, I would like this to be a warning, not a hard error, but Studio is a tool for normal people so showing a random warning in the UI is not helpful. And Studio cannot distinguish between a normal user and an admin that is trying things. I would also think it's not outlandish to expect admins to check the dev console? Especially since the docs recommend doing that.

So yeah: I see the problem, but given the things I mentioned, I gravitate towards keeping the status quo. But I'm still open to have my mind changed.

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 a pull request may close this issue.

2 participants