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 serviceType for the collector service #1286

Merged

Conversation

sschne
Copy link
Contributor

@sschne sschne commented Oct 31, 2020

Adds serviceType option for the collector service.

Closes: #1263

Signed-off-by: Simon Schneider github@simon-schneider.eu

Signed-off-by: Simon Schneider <github@simon-schneider.eu>
@codecov
Copy link

codecov bot commented Oct 31, 2020

Codecov Report

Merging #1286 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1286   +/-   ##
=======================================
  Coverage   87.32%   87.33%           
=======================================
  Files          89       89           
  Lines        4947     4957   +10     
=======================================
+ Hits         4320     4329    +9     
- Misses        464      465    +1     
  Partials      163      163           
Impacted Files Coverage Δ
pkg/apis/jaegertracing/v1/jaeger_types.go 100.00% <ø> (ø)
pkg/service/collector.go 100.00% <100.00%> (ø)
pkg/controller/jaeger/jaeger_controller.go 36.87% <0.00%> (+1.23%) ⬆️

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 c418931...0867f39. Read the comment docs.

Copy link
Contributor

@jpkrohling jpkrohling 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! Could we have an example under /examples, showing how to use it? People ask about this quite often. It would also be good to have this new example as part of the e2e tests, so that we can document whether this works in non-vanilla Kubernetes (OpenShift, for instance).

Signed-off-by: Simon Schneider <github@simon-schneider.eu>
@sschne sschne force-pushed the feature/collector-service-type branch from 7583918 to 0867f39 Compare November 5, 2020 21:10
Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

LGTM. @kevinearls, would you please check whether the example test looks sane to you?

@jpkrohling
Copy link
Contributor

By the way: I just confirmed that the new test is being executed and is passing:

2020-11-05T21:29:55.6721753Z     --- PASS: TestExamplesSuite2/TestServiceTypesExample (51.45s)

Copy link
Contributor

@kevinearls kevinearls left a comment

Choose a reason for hiding this comment

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

The test changes LGTM.

@sschne
Copy link
Contributor Author

sschne commented Nov 10, 2020

Is there anything missing for this PR?

@jpkrohling jpkrohling merged commit 37135a4 into jaegertracing:master Nov 11, 2020
@jpkrohling
Copy link
Contributor

No, I was expecting mergify to merge it, but for some reason, it didn't (codecov checks missing?). I merged this manually.

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.

Allow service types to be configured
3 participants