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.

Sign up for GitHub

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

Jump to bottom

Allow configuring CORS origins in config file #811

Merged
merged 9 commits into from
Mar 16, 2023
Merged

Allow configuring CORS origins in config file #811

merged 9 commits into from
Mar 16, 2023

Conversation

baont
Copy link
Contributor

@baont baont commented Feb 23, 2023 •
edited by minwoox
Loading

This is my implementation for the issue #754.

Motivation
Provide a way to configure CORS policies, so that web applications hosted in a different origin can call Central Dogma REST API.

Modifications

  • Add corsAllowedOrigins to CentralDogmaConfig
  • Add Cors decorator when setting up HttpApi

Result:

@CLAassistant
Copy link

CLAassistant commented Feb 23, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Feb 23, 2023 •
edited
Loading

Codecov Report

Patch coverage: 86.84% and project coverage change: -0.11 ⚠️

Comparison is base (bda3bec) 65.71% compared to head (84a7a94) 65.61%.

Additional details and impacted files 
@@             Coverage Diff              @@
##             master     #811      +/-   ##
============================================
- Coverage     65.71%   65.61%   -0.11%     
- Complexity     3289     3319      +30     
============================================
  Files           350      355       +5     
  Lines         13731    13852     +121     
  Branches       1488     1498      +10     
============================================
+ Hits           9024     9089      +65     
- Misses         3863     3917      +54     
- Partials        844      846       +2
Impacted Files Coverage Δ
...a/com/linecorp/centraldogma/server/CorsConfig.java 78.26% <78.26%> (ø)
...com/linecorp/centraldogma/server/CentralDogma.java 74.61% <100.00%> (+0.57%) ⬆️
...ecorp/centraldogma/server/CentralDogmaBuilder.java 54.19% <100.00%> (+0.71%) ⬆️
...necorp/centraldogma/server/CentralDogmaConfig.java 80.95% <100.00%> (+0.26%) ⬆️

... and 12 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@baont baont force-pushed the config_cors branch 2 times, most recently from 72ee254 to 99e21e4 Compare March 1, 2023 02:11
@baont baont marked this pull request as ready for review March 1, 2023 03:22
jrhee17
jrhee17 reviewed Mar 3, 2023
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Looks good! I'm guessing with the next iteration of reviews this PR should be good for merging 👍

it/src/test/java/com/linecorp/centraldogma/it/CorsPolicyTest.java Outdated Show resolved Hide resolved
it/src/test/java/com/linecorp/centraldogma/it/CorsPolicyTest.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaBuilder.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaConfig.java Outdated Show resolved Hide resolved
site/src/sphinx/setup-configuration.rst Outdated Show resolved Hide resolved
@baont
- allow specify maxAge
4c3232b 
- allow using a string or an array of strings to specify cors allowed origins
- fix code style
- use assertJ in tests
ikhoon
ikhoon reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Left only minor comments.

it/src/test/java/com/linecorp/centraldogma/it/CorsPolicyTest.java Outdated Show resolved Hide resolved
it/src/test/java/com/linecorp/centraldogma/it/CorsPolicyTest.java Outdated Show resolved Hide resolved
it/src/test/java/com/linecorp/centraldogma/it/CorsPolicyTest.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CorsConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CorsConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CorsConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaBuilder.java Outdated Show resolved Hide resolved
site/src/sphinx/setup-configuration.rst Outdated Show resolved Hide resolved
server/src/test/java/com/linecorp/centraldogma/server/CentralDogmaConfigTest.java Outdated Show resolved Hide resolved
server/src/test/java/com/linecorp/centraldogma/server/CentralDogmaConfigTest.java Outdated Show resolved Hide resolved
@baont
- fix code style
eec7648 
- don't allow unpositive maxAge
- fix doc
ikhoon
ikhoon approved these changes Mar 7, 2023
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks, @baont! 🚀💯
Left a minor comment.

server/src/main/java/com/linecorp/centraldogma/server/CorsConfig.java Outdated Show resolved Hide resolved
@ikhoon ikhoon added this to the 0.60.2 milestone Mar 7, 2023
jrhee17
jrhee17 reviewed Mar 7, 2023
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

I think this will be my last review. Looking really nice 👍 Left only nit comments 🙇

site/src/sphinx/setup-configuration.rst Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CorsConfig.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java Outdated Show resolved Hide resolved
site/src/sphinx/setup-configuration.rst Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CorsConfig.java Outdated Show resolved Hide resolved
jrhee17
jrhee17 approved these changes Mar 8, 2023
View reviewed changes
Copy link
Contributor

@jrhee17 jrhee17 left a comment

Choose a reason for hiding this comment

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

Thanks for your patience @baont ! Looks good to me 👍 🚀 💯

server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaBuilder.java Outdated Show resolved Hide resolved
server/src/main/java/com/linecorp/centraldogma/server/CentralDogmaBuilder.java Outdated Show resolved Hide resolved
@baont @jrhee17
Apply suggestions from code review
acbd869 
Co-authored-by: jrhee17 <guins_j@guins.org>
trustin
trustin approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Excellent work, @baont! 🙇

server/src/main/java/com/linecorp/centraldogma/server/CentralDogma.java Outdated Show resolved Hide resolved
@jrhee17 jrhee17 mentioned this pull request Mar 9, 2023
Closed
@jrhee17
Copy link
Contributor

jrhee17 commented Mar 9, 2023

gentle ping @minwoox 😄

@jrhee17
Copy link
Contributor

jrhee17 commented Mar 13, 2023

gentle ping @minwoox 😄

minwoox
minwoox approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

Sorry about the late review. Thanks a lot for adding this configuration! 😄

it/src/test/java/com/linecorp/centraldogma/it/CorsPolicyTest.java Outdated Show resolved Hide resolved
@minwoox minwoox changed the title allow config CORS origins in config file Allow configuring CORS origins in config file Mar 16, 2023
@minwoox minwoox merged commit 9fbb0b6 into line:master Mar 16, 2023
@minwoox
Copy link
Member

minwoox commented Mar 16, 2023

Thanks a lot, @baont! 🎉 🎉 🎉

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

@trustin trustin trustin approved these changes

@ikhoon ikhoon ikhoon approved these changes

@jrhee17 jrhee17 jrhee17 approved these changes

@minwoox minwoox minwoox approved these changes

Assignees
No one assigned
Labels
new feature
Projects
None yet
Milestone
0.61.0
Development

Successfully merging this pull request may close these issues.

Provide a way to configure CORS policies
6 participants
@baont @CLAassistant @jrhee17 @minwoox @trustin @ikhoon