-
Notifications
You must be signed in to change notification settings - Fork 194
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
Use Elasticsearch configuration file to avoid tricky envvar names #4992
Conversation
…able names It appears that there is no way to pass allow-origin as an environment variable, as ES will not recognise the same option with an underscore, and provides no method to map hyphens. The configuration file is much more searchable against the docs anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approach taken looks correct and I do think ultimately this is a better approach even if it was forced on us by docker compose.
One caveat to my review is that I am away from my computer and cannot run this locally. It does look right though, so feel free to use your discretion in merging.
I'll merge based on that review, my testing locally using the tests I shared in the PR description, and if CI passes, which includes running API tests and loading sample data, both of which use the local Elasticsearch cluster. |
I'm looking now as well, if you wanted a second set of eyes before merging. |
Ah, sorry @AetherUnbound, your comment had not come through on the page! If you find any issues I am around to hotfix. |
No worries, changes look good to me! |
Fixes
Fixes #4974 by @zackkrida
Description
See my comment on the issue clarifying the cause and long-term fixes for additional rationale of this change.
TL;DR: Docker compose broke the way environment variables are parsed, and no longer accepts those defined in
es/env.docker
. On the other hand, it appears that there is no way to pass allow-origin as an environment variable, as ES will not recognise the same option with an underscore, and provides no method to map hyphens. The configuration file is much more searchable against the docs anyway.Testing Instructions
CI must pass (proving ES is accessible throughout the development stack). Also check out this branch and run
ov j api/up
to get ES running locally, and make an http request likeov http http://0.0.0.0:50292/_cluster/health
. Also use a tool like Elasticvue to prove CORS is working for local development usage.Checklist
Update index.md
).main
) or a parent feature branch.ov just catalog/generate-docs
for catalogPRs) or the media properties generator (
ov just catalog/generate-docs media-props
for the catalog or
ov just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin