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

Feature/add cors headers #19

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ben-hamelin
Copy link

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

@rfay
Copy link
Member

rfay commented Nov 7, 2023

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.

Comment on lines +18 to +22
- "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"
Copy link
Member

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?

Copy link
Author

Choose a reason for hiding this comment

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

@rfay
Copy link
Member

rfay commented Nov 8, 2023

Yes...

This is the simplest option. Enable CORS on Elasticsearch by adding the following to your elasticsearch.yml file:

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.

@ben-hamelin
Copy link
Author

@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.

@rfay
Copy link
Member

rfay commented Apr 18, 2024

@AronNovak is the maintainer here, hopefully we can raise him :)

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 this pull request may close these issues.

2 participants