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.

Sign up for GitHub

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

feat: Add a plausibility check for Meilisearch configuration #1186

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: Add a plausibility check for Meilisearch configuration #1186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/20250102_163724_fghaas_check_meilisearch.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- [Improvement] Add a plausibility check that issues a warning if `RUN_MEILISEARCH` is `false` but `MEILISEARCH_URL` is unset. (by @fghaas)
20 changes: 20 additions & 0 deletions tutor/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,3 +358,23 @@ def _check_preview_lms_host(config: Config) -> None:
f'Warning: PREVIEW_LMS_HOST="{preview_lms_host}" is not a subdomain of LMS_HOST="{lms_host}". '
"This configuration is not typically recommended and may lead to unexpected behavior."
)


@hooks.Actions.CONFIG_LOADED.add()
def _check_run_meilisearch(config: Config) -> None:
"""
In case RUN_MEILISEARCH is set to false, check if
MEILISEARCH_URL has been set to a non-default value.
If not, print a warning to notify the user.
"""

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"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to avoid making another call to get_defaults here, which is slow. Can we just hardcode "meilisearch"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Your approach: calling get_defaults(), which is slow.
  2. Implementing a CONFIG_DEFAULTS_LOADED action 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.

if meilisearch_url == meilisearch_url_default:
fmt.echo_alert(
"Warning: RUN_MEILISEARCH is false, but MEILISEARCH_URL is unset. "
"This configuration is not typically recommended and may lead to unexpected behavior. "
"Consider setting RUN_MEILISEARCH, or setting MEILISEARCH_URL to an existing, preconfigured Meilisearch instance."
)
Loading