feat: Add a plausibility check for Meilisearch configuration#1186
feat: Add a plausibility check for Meilisearch configuration#1186fghaas wants to merge 1 commit intooverhangio:mainfrom
Conversation
Tutor is meant to be configured with either its own, or an externally preconfigured Meilisearch instance. Thus, the combination of configuring RUN_MEILISEARCH: false and leaving MEILISEARCH_URL at its default is almost certainly a configuration error. Add an alert to notify the user of that configuration issue. Related issue: overhangio#1185
fghaas
force-pushed
the
check-meilisearch
branch
from
58aeb64 to
768c893
Compare
regisb
left a comment
regisb
left a comment
There was a problem hiding this comment.
This looks good, with just a minor comment. Could you please rebase on top of the release branch, such that current sumac users can benefit from the change?
| run_meilisearch = get_typed(config, "RUN_MEILISEARCH", bool, True) | ||
| if not run_meilisearch: | ||
| meilisearch_url = get_typed(config, "MEILISEARCH_URL", str, "") | ||
| meilisearch_url_default = get_defaults()["MEILISEARCH_URL"] |
There was a problem hiding this comment.
I'd like to avoid making another call to get_defaults here, which is slow. Can we just hardcode "meilisearch"?
There was a problem hiding this comment.
I'm never a big fan of hardcoding the values of defaults, because it means that if the default ever changes, you need to fix it in more than one place. Is there another (better) way to avoid the get_defaults() call?
There was a problem hiding this comment.
I'm never a big fan of hardcoding the values of defaults, because it means that if the default ever changes, you need to fix it in more than one place.
[thoughts] While I agree with this, I try to weigh the effort involved in determining how many "changes" are needed. If it is needed in very few places, it can be okay to hardcode.
There was a problem hiding this comment.
I do agree with you Florian that hard-coding values is typically a bad practice. There are other ways to bypass this, but I wouldn't say they are "better". They include:
- Your approach: calling
get_defaults(), which is slow. - Implementing a
CONFIG_DEFAULTS_LOADEDaction that would save a singleton copy of the (unrendered) config defaults, which would be quite complex.
If we ever forget to modify the meilisearch URL default, the warning you added will be present in the console stderr. Also, to make sure we don't forget to update this value, you could add a unit test.
There was a problem hiding this comment.
@fghaas Could you please share your availability to work on this? If you’re unavailable or prefer not to continue with it, we can reassign it to someone else.
4 participants
Tutor is meant to be configured with either its own, or an externally preconfigured Meilisearch instance. Thus, the combination of configuring
RUN_MEILISEARCH: falseand leavingMEILISEARCH_URLat its default is almost certainly a configuration error.Add an alert to notify the user of that configuration issue.
Related issue:
#1185