Add support for NodePort in Jaeger Query Service #1394
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1394 +/- ##
==========================================
+ Coverage 86.70% 86.73% +0.03%
==========================================
Files 90 91 +1
Lines 5017 5059 +42
==========================================
+ Hits 4350 4388 +38
- Misses 513 516 +3
- Partials 154 155 +1
Continue to review full report at Codecov.
|
jpkrohling
changed the title
|
@rubenvp8510 , would you please review this one? |
pkg/service/query.go
Outdated
| @@ -90,3 +95,8 @@ func getTypeForQueryService(jaeger *v1.Jaeger) corev1.ServiceType { | |||
| } | |||
| return corev1.ServiceTypeClusterIP | |||
| } | |||
|
|
|||
| // GetNodePortForQueryService returns the query service NodePort for this Jaeger instance | |||
| func GetNodePortForQueryService(jaeger *v1.Jaeger) int { | |||
There was a problem hiding this comment.
Why this function is returning int instead of int32. When you use it you cast it again to int32
I would suggest to change this to:
func GetNodePortForQueryService(jaeger *v1.Jaeger) int32 {
return jaeger.Spec.Query.NodePort
}
And ged rid of all int32(GetNodePortForQueryService(jaeger)) casts.
There was a problem hiding this comment.
Hi -
I set the function to return an int in lieu of an int32, for consistency with the getTargetPortForQueryService function. Let me make the change and update this MR 👍
|
Small comments, overall looks good. |
Signed-off-by: GitHub <[email protected]>
Signed-off-by: GitHub <[email protected]>
This PR should allow users to expose Jaeger's Query service as a NodePort at a specific port. Earlier, the Jaeger Operator did not support specifying a port value and would depend on K8s to randomly select an apt port value. This should resolve this blocker.
Resolves #1307