Skip to content
This repository was archived by the owner on Jul 16, 2021. It is now read-only.

Enable configuring CORS to remove same-domain requirement#266

Merged
prydonius merged 2 commits into
helm:masterfrom
prydonius:cors-configuration
Jun 7, 2017
Merged

Enable configuring CORS to remove same-domain requirement#266
prydonius merged 2 commits into
helm:masterfrom
prydonius:cors-configuration

Conversation

@prydonius
Copy link
Copy Markdown
Member

@prydonius prydonius commented Jun 6, 2017

fixes #235

googleAnalyticsId: '{{.Values.ui.googleAnalyticsId}}',
appName: '{{.Values.ui.appName}}',
{{- if .Values.ui.backendHostname }}
backendHostname: '{{ .Values.ui.backendHostname }}',
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I decided not to make the default /api to not change the way it currently work, but maybe that makes sense?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it makes sense to remove it because we can use a subdomain like api.mydomain.com. Adding /api may be repetitive. However, I think this is not so important now and it's a different issue.

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #266 into master will increase coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #266      +/-   ##
==========================================
+ Coverage   90.14%   90.33%   +0.18%     
==========================================
  Files          18       18              
  Lines         944      962      +18     
==========================================
+ Hits          851      869      +18     
  Misses         62       62              
  Partials       31       31
Impacted Files Coverage Δ
src/api/config/cors/cors.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c663e62...e7cb88c. Read the comment docs.

@prydonius prydonius merged commit 7018415 into helm:master Jun 7, 2017
@prydonius prydonius deleted the cors-configuration branch June 7, 2017 13:42
googleAnalyticsId: '{{.Values.ui.googleAnalyticsId}}',
appName: '{{.Values.ui.appName}}',
{{- if .Values.ui.backendHostname }}
backendHostname: '{{ .Values.ui.backendHostname }}',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it makes sense to remove it because we can use a subdomain like api.mydomain.com. Adding /api may be repetitive. However, I think this is not so important now and it's a different issue.

# source: my-repository-source
cors:
allowed_origins:
- my-api-server
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see this is a configurable parameter. Can you add some documentation? I'm not sure if the frontend will work with this default configuration.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the default in https://github.com/helm/monocular/blob/master/src/api/config/cors/cors.go#L30, I just copied it here. It means that only a hostname of my-api-server or same-origin will work. A blank value would allow all origins, I believe.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You're right 👍. Thanks for the explanation

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support Helm deployment without Ingress controller

3 participants