-
-
Notifications
You must be signed in to change notification settings - Fork 15
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
Feature/add cors headers #19
base: main
Are you sure you want to change the base?
Conversation
Since you're a new contributor, the tests have to be allowed manually on this first PR to this repo. Ping me when you push new things and I'll allow again. |
- "http.cors.allow-origin=*" | ||
- "http.cors.enabled=true" | ||
- "http.cors.allow-headers=X-Requested-With,X-Auth-Token,Content-Type,Content-Length,Authorization,Access-Control-Allow-Origin,Access-Control-Request-Headers" | ||
- "http.cors.allow-credentials=true" | ||
- "http.cors.allow-methods: OPTIONS, HEAD, GET, POST, PUT, DELETE" |
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.
Are these environment variables? Or some other type of configuration? Dots in environment variables are unusual. And who consumes this information?
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.
This is configuration used by ES, based on documentation here: https://www.elastic.co/guide/en/elasticsearch/reference/current/behavioral-analytics-cors.html#behavioral-analytics-cors-enable-cors-elasticsearch
Yes...
They aren't environment variables, they have to be added to elasticsearch.yml. I'll bet they could also be added as command-line arguments to the elasticsearch daemon. |
@rfay Following up on this. I can confirm the changes here work locally to resolve CORS errors. Would you prefer an updated approach that adds a "volumes" mapping to https://github.com/ddev/ddev-elasticsearch/blob/main/docker-compose.elasticsearch.yaml#L21 and a new elasticsearch7.yml file? I would review and apply the same settings for 8. |
@AronNovak is the maintainer here, hopefully we can raise him :) |
The Issue
#17
How This PR Solves The Issue
Adds http.cors configuration to the compose file
Manual Testing Instructions
Make a Javascript "fetch()" request from either a decoupled localhost environment, or from any other locally hosted webpage, could even be static HTML.
Automated Testing Overview
I am reviewing the tests now to see if there are any that require an update, or if it is appropriate to create a new one
Related Issue Link(s)
Release/Deployment Notes