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

Add CORS handling for Zipkin collector service, fixes #703 #1463

Merged
merged 6 commits into from Apr 16, 2019
Merged

Add CORS handling for Zipkin collector service, fixes #703 #1463

merged 6 commits into from Apr 16, 2019

Conversation

JonasVerhofste
Copy link
Contributor

Signed-off-by: Jonas Verhofsté 25819942+JonasVerhofste@users.noreply.github.com

Which problem is this PR solving?

Short description of the changes

  • Add and call the CORS handler
  • Add variables for allowed origins and headers

@JonasVerhofste
Copy link
Contributor Author

Not entirely sure where to add tests, if any are needed at all?

@yurishkuro
Copy link
Member

I don't think this PR needs to include Gopkg.lock changes.

Not entirely sure where to add tests, if any are needed at all?

What is the expected behavior of the cors handler? Does it reject certain types of requests? That's the type of test I would add.

@codecov
Copy link

codecov bot commented Apr 6, 2019

Codecov Report

Merging #1463 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1463      +/-   ##
==========================================
+ Coverage   99.83%   99.83%   +<.01%     
==========================================
  Files         179      179              
  Lines        8550     8554       +4     
==========================================
+ Hits         8536     8540       +4     
  Misses          7        7              
  Partials        7        7
Impacted Files Coverage Δ
cmd/collector/app/builder/builder_flags.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 0b9311d...9b98721. Read the comment docs.

@JonasVerhofste
Copy link
Contributor Author

JonasVerhofste commented Apr 8, 2019

I don't think this PR needs to include Gopkg.lock changes.

Removed.

What is the expected behavior of the cors handler? Does it reject certain types of requests? That's the type of test I would add.

I'm not sure where to add the tests. Where exactly does the main.go from the collector get tested?

@yurishkuro
Copy link
Member

Ah, it's ok then. We should have a test for flags, but we don't test main aside from integration tests. Some steps in the build are failing though.

@JonasVerhofste
Copy link
Contributor Author

@yurishkuro Well, the builds didn't fail before (https://travis-ci.org/jaegertracing/jaeger/builds/516424159) and I only removed the lock-file. Some inconsistency in the CI?

Signed-off-by: Jonas Verhofsté <25819942+JonasVerhofste@users.noreply.github.com>
@JonasVerhofste
Copy link
Contributor Author

@yurishkuro So, seems like build 2 (proto gen) is failing because I removed Gopkg.lock, like you asked. Should I re-add it or is that a fault in the CI?

@JonasVerhofste
Copy link
Contributor Author

ping @yurishkuro

@yurishkuro
Copy link
Member

Let's see if merging master helps, as this step should not be failing (it doesn't in master)

@yurishkuro
Copy link
Member

hmm, very odd, lock file comes up as a diff. Need to look into that.

Yuri Shkuro added 2 commits April 16, 2019 00:29
Signed-off-by: Yuri Shkuro <ys@uber.com>
Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro
Copy link
Member

ok, fixed it

Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro
Copy link
Member

Ah, I see what's going on now. You're adding a new dependency github.com/rs/cors, I missed that. In that case, it needs to go to Gopkg.toml, and you DO need to update the lock file.

Signed-off-by: Yuri Shkuro <ys@uber.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants