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

Handle CORS requests #6289

Merged
merged 30 commits into from
May 13, 2019
Merged

Handle CORS requests #6289

merged 30 commits into from
May 13, 2019

Conversation

tamalsaha
Copy link
Contributor

@tamalsaha tamalsaha commented Mar 9, 2019

Fixes #5724

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 9, 2019
@techknowlogick techknowlogick added type/enhancement An improvement of existing functionality modifies/api This PR adds API routes or modifies them labels Mar 9, 2019
@techknowlogick techknowlogick added this to the 1.9.0 milestone Mar 9, 2019
Signed-off-by: Tamal Saha <tamal@appscode.com>
@codecov-io
Copy link

codecov-io commented Mar 9, 2019

Codecov Report

Merging #6289 into master will increase coverage by 0.01%.
The diff coverage is 81.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6289      +/-   ##
=========================================
+ Coverage   41.38%   41.4%   +0.01%     
=========================================
  Files         440     441       +1     
  Lines       59703   59736      +33     
=========================================
+ Hits        24709   24734      +25     
- Misses      31758   31762       +4     
- Partials     3236    3240       +4
Impacted Files Coverage Δ
modules/setting/setting.go 47.67% <100%> (+0.09%) ⬆️
routers/api/v1/api.go 70.56% <76.92%> (+0.13%) ⬆️
modules/setting/cors.go 84.21% <84.21%> (ø)
modules/log/event.go 64.46% <0%> (-1.53%) ⬇️
models/repo_list.go 66.84% <0%> (-1.06%) ⬇️
models/gpg_key.go 55.83% <0%> (-0.84%) ⬇️
routers/repo/view.go 43.03% <0%> (+1.01%) ⬆️
models/unit.go 67.56% <0%> (+5.4%) ⬆️

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 6fb58a8...0c56e5f. Read the comment docs.

Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

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

A few more items to be addressed.

@techknowlogick
Copy link
Member

@tamalsaha could you resolve git conflicts, update to use go modules, add tests, and update per feedback? If you do not have any available time to do so please let us know and I can take over this PR to make the required changes.

@tamalsaha
Copy link
Contributor Author

@techknowlogick, please feel free to make changes as you see fit. My bandwidth is tight for next few weeks. Thanks!

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 18, 2019
go.mod Outdated
@@ -136,5 +137,6 @@ require (

replace (
github.com/denisenkom/go-mssqldb v0.0.0-20181014144952-4e0d7dc8888f => github.com/denisenkom/go-mssqldb v0.0.0-20161128230840-e32ca5036449
github.com/go-macaron/cors v0.0.0-20190309005821-1733aeedd68a => github.com/tamalsaha/cors v0.0.0-20190309005821-1733aeedd68a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is this replace needed? https://github.com/go-macaron/cors is up-todate with github.com/tamalsaha/cors

Copy link
Member

Choose a reason for hiding this comment

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

See: go-macaron/cors#1
it fails otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@techknowlogick , I just updated go-macaron/cors@6fd6a9b . Please revendor . This should be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fix 😄

@tamalsaha
Copy link
Contributor Author

@techknowlogick , do you plan to make any more changes? I wonder when this will be merged.

@techknowlogick
Copy link
Member

@tamalsaha my changes are done, and only one other review is needed. Once a 2nd LG-TM is given then this can be merged.

@FallingSnow
Copy link

Is there anything we can do to accelerate a 2nd LG-TM?

@lunny
Copy link
Member

lunny commented May 11, 2019

Please resolve the conflicts.

# Conflicts:
#	Gopkg.lock
#	routers/api/v1/api.go
@@ -135,6 +136,6 @@ require (
)

replace (
github.com/denisenkom/go-mssqldb v0.0.0-20181014144952-4e0d7dc8888f => github.com/denisenkom/go-mssqldb v0.0.0-20161128230840-e32ca5036449
github.com/go-sql-driver/mysql v1.4.0 => github.com/go-sql-driver/mysql v0.0.0-20181218123637-c45f530f8e7f
github.com/denisenkom/go-mssqldb => github.com/denisenkom/go-mssqldb v0.0.0-20161128230840-e32ca5036449
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tamalsaha
Copy link
Contributor Author

@lunny , I have fixed the conflicts. PTAL.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 13, 2019
@techknowlogick techknowlogick merged commit 34d06f4 into go-gitea:master May 13, 2019
@tamalsaha tamalsaha deleted the cors-pr branch May 13, 2019 15:52
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CORS preflight OPTIONS request to API returns 404
6 participants