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

Change Zipkin CORS origins and headers to comma separated list #1556

Merged
merged 1 commit into from Aug 15, 2019

Conversation

@JonasVerhofste
Copy link
Contributor

JonasVerhofste commented May 21, 2019

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

Which problem is this PR solving?

When I implemented the CORS handling in f9702c4 (#1463), I forgot that there can be more than one origin and one header. This adds the functionality of specifying multiple origins and headers as a comma separated list.

@codecov

This comment has been minimized.

Copy link

codecov bot commented May 21, 2019

Codecov Report

Merging #1556 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1556      +/-   ##
==========================================
- Coverage   98.36%   98.32%   -0.05%     
==========================================
  Files         193      193              
  Lines        9364     9364              
==========================================
- Hits         9211     9207       -4     
- Misses        119      122       +3     
- Partials       34       35       +1
Impacted Files Coverage Δ
cmd/collector/app/builder/builder_flags.go 100% <100%> (ø) ⬆️
cmd/query/app/static_handler.go 83.33% <0%> (-3.51%) ⬇️

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 75b670f...47c720b. Read the comment docs.

Copy link
Member

yurishkuro left a comment

Could you add tests?

@JonasVerhofste

This comment has been minimized.

Copy link
Contributor Author

JonasVerhofste commented May 21, 2019

@yurishkuro Well, it's just an edit of what I added in #1463, where you told me there are no tests? If I do want me to write some, could you point me in the direction of where I'm supposed to add them?

@JonasVerhofste

This comment has been minimized.

Copy link
Contributor Author

JonasVerhofste commented May 21, 2019

I also based myself on how arrays are handled in the storage plugins, but I'm not sure if this is the right way. Can Viper handle arrays like this? For example, in yaml:

collector:
  zipkin:
    allowed-origins:
      - origin1
      - origin2

As far as I understand the format above is not going to work, but this format would:

collector:
  zipkin:
    allowed-origins: "origin1, origin2"

I could be wrong of course, haven't gotten the time yet to test it, it's just all just how I interpret the code. Although if I am right, I would suggest taking a look if we could somehow use both (to provide backwards compatibility). The first example is much more human readable than the second one, in my opinion. If I'm wrong, do correct me! Can always have missed something.

@yurishkuro

This comment has been minimized.

Copy link
Member

yurishkuro commented May 21, 2019

I don't know about your Viper question, you'd have to try.

My earlier comment about no-testing was in relation to CORS functionality. But we do have tests for parsing command line parameters:

$ go test -cover ./cmd/collector/app/builder/
ok  	github.com/jaegertracing/jaeger/cmd/collector/app/builder	0.019s	coverage: 100.0% of statements
@JonasVerhofste

This comment has been minimized.

Copy link
Contributor Author

JonasVerhofste commented May 24, 2019

I don't know about your Viper question, you'd have to try.

It seems I was right, only the second one works, arrays don't work. I'll see if I can fix that.

But we do have tests for parsing command line parameters:

@yurishkuro the only test I can see there is span_handler_builder_test.go, as far as I can tell, there's no test for the main.go or the builder_flags.go, which are the only files I've edited (so far).

@@ -225,10 +226,13 @@ func startZipkinHTTPAPI(
r := mux.NewRouter()
zHandler.RegisterRoutes(r)

origins := strings.Split(strings.Replace(allowedOrigins, " ", "", -1), ",")
headers := strings.Split(strings.Replace(allowedHeaders, " ", "", -1), ",")

This comment has been minimized.

Copy link
@yurishkuro

yurishkuro May 27, 2019

Member

we shouldn't do this in main, it is better to encapsulate this into the builder, similar to this:

hostPorts := v.GetString(collectorHostPort)
if hostPorts != "" {
b.CollectorHostPorts = strings.Split(hostPorts, ",")
}

@yurishkuro

This comment has been minimized.

Copy link
Member

yurishkuro commented Aug 10, 2019

@JonasVerhofste do you plan to finish this one?

@JonasVerhofste

This comment has been minimized.

Copy link
Contributor Author

JonasVerhofste commented Aug 12, 2019

@yurishkuro Sure, but I'm still wondering where I should add those tests. Like I noted before, cmd/collector/app/builder/builder_flags.go doesn't have a test file. Whatever I change to the file, the coverage stays the same. Which is probably because only cmd/collector/app/builder/span_handler_builder.go has a test-file?

Signed-off-by: Yuri Shkuro <ys@uber.com>
@yurishkuro yurishkuro merged commit 581f9be into jaegertracing:master Aug 15, 2019
5 checks passed
5 checks passed
DCO DCO
Details
WIP Ready for review
Details
codecov/patch 100% of diff hit (target 95%)
Details
codecov/project 98.32% (target 95%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.