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

Added CORS policy for tts get request #634

Merged
merged 4 commits into from Sep 16, 2020
Merged

Added CORS policy for tts get request #634

merged 4 commits into from Sep 16, 2020

Conversation

insualk
Copy link
Contributor

@insualk insualk commented Sep 16, 2020

Issue: #633

@@ -26,6 +26,10 @@ public void ConfigureServices(IServiceCollection services)
builder.AllowAnyOrigin().AllowAnyMethod().AllowAnyHeader().WithExposedHeaders(
"Grpc-Status", "Grpc-Message", "Grpc-Encoding", "Grpc-Accept-Encoding");
}));
services.AddCors(o => o.AddPolicy("AllowAudioFileGet", builder =>
Copy link
Collaborator

Choose a reason for hiding this comment

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

merge with the previous call to services.AddCors(?

i.e.

services.AddCors(o => {
  o.AddPolicy...
  o.AddPolicy...

Copy link
Collaborator

@ztl8702 ztl8702 Sep 16, 2020

Choose a reason for hiding this comment

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

Or, I think you can just reuse the AllowAll policy (it's more lenient than AllowAudioFileGet)

@ztl8702
Copy link
Collaborator

ztl8702 commented Sep 16, 2020

p.s. should we exclude server/backend/.vscode?

No. I think we should check .vscode/ into git. I think that's what most projects are doing, including VSCode itself.

@ztl8702
Copy link
Collaborator

ztl8702 commented Sep 16, 2020

Nice work! Free free to merge.

@ztl8702
Copy link
Collaborator

ztl8702 commented Sep 16, 2020

tip:

We have mergify bot configured to automerge a PR when tests pass.

You can write the commit message in a subheading called Commit message in the PR description (example ), and tag the PR with automerge. Mergify will handle the rest for you.

@insualk
Copy link
Contributor Author

insualk commented Sep 16, 2020

tip:

We have mergify bot configured to automerge a PR when tests pass.

You can write the commit message in a subheading called Commit message in the PR description (example ), and tag the PR with automerge. Mergify will handle the rest for you.

What's the purpose of adding the commit message subheading?

@insualk insualk added the automerge Merges without approvals (after tests pass) label Sep 16, 2020
@ztl8702
Copy link
Collaborator

ztl8702 commented Sep 16, 2020

What's the purpose of adding the commit message subheading?

The default commit message generated by github is not always what you'd expect. Since we ask mergify to merge automatically, we want to tell mergify exactly what commit message to use, rather than using the github default.

According to mergify docs: "you can override the default commit message... add a section in the pull request body that starts with Commit Message."

https://doc.mergify.io/actions.html#commit-message

@ztl8702 ztl8702 changed the title allow CORS for tts get request from any origin Added CORS policy for tts get request Sep 16, 2020
@ztl8702
Copy link
Collaborator

ztl8702 commented Sep 16, 2020

I just switched the automerge bot from mergify to kodiak, which doesn't require you to add a "Commit Message" subheading anymore. @insualk

@kodiakhq kodiakhq bot merged commit c82cecb into master Sep 16, 2020
@kodiakhq kodiakhq bot deleted the nixl/tts-CORS branch September 16, 2020 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Merges without approvals (after tests pass)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants