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

Fix metrics auth token detection #6006

Merged
merged 3 commits into from
Feb 9, 2019
Merged

Conversation

barkap
Copy link
Contributor

@barkap barkap commented Feb 8, 2019

Metrics token authorization currently does not work with token and metrics enabled in config. There's always 401 response without any data. This fixes header detection.

Signed-off-by: Pauls Barkans <paulsb@gmail.com>
@techknowlogick
Copy link
Member

Thanks for PR @barkap, would you be able to add a test for this?

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 8, 2019
@techknowlogick techknowlogick added this to the 1.8.0 milestone Feb 8, 2019
@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 Feb 8, 2019
@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 Feb 8, 2019
@codecov-io
Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #6006 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6006   +/-   ##
=======================================
  Coverage   38.72%   38.72%           
=======================================
  Files         332      332           
  Lines       48992    48992           
=======================================
  Hits        18973    18973           
  Misses      27271    27271           
  Partials     2748     2748
Impacted Files Coverage Δ
routers/metrics.go 0% <0%> (ø) ⬆️

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 11a9ce6...aa87281. Read the comment docs.

@barkap
Copy link
Contributor Author

barkap commented Feb 8, 2019

Thanks for PR @barkap, would you be able to add a test for this?

I'm not too familiar with gitea as a whole and existing go tests mostly fail for me so I'm probably unable to write new test with mocks for this package at the time.

@techknowlogick techknowlogick merged commit 8c865f3 into go-gitea:master Feb 9, 2019
@techknowlogick
Copy link
Member

@barkap that's ok, thanks for explaining. My computer is so low powered that I rely on drone for running my tests as otherwise it'd be >1.5hours just to test against sqlite 😢 maybe it is time for me to get a new computer 🤔

Would you be able to send a backport of this PR against the release/v1.7 branch? If you need any clarification or assistance please let me know and I can help you out.

@barkap
Copy link
Contributor Author

barkap commented Feb 9, 2019

Yes, I'll send pr in a bit. Thank you.

barkap added a commit to barkap/gitea that referenced this pull request Feb 9, 2019
Signed-off-by: Pauls Barkans <paulsb@gmail.com>
@lafriks lafriks added the backport/done All backports for this PR have been created label Feb 9, 2019
zeripath pushed a commit that referenced this pull request Feb 9, 2019
Backport of #6006 

Signed-off-by: Pauls Barkans <paulsb@gmail.com>
@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
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants