jaegertracing / helm-charts

Helm Charts for Jaeger backend
Apache License 2.0
254 stars 338 forks source link

[jaeger] Add appProtocol #539 #540

Closed antonioberben closed 4 months ago

antonioberben commented 5 months ago

What this PR does

Which issue this PR fixes

Checklist

pavelnikolov commented 4 months ago

@antonioberben This would fail if the K8s version doesn't support it. To ensure backwards compatibility, please, check if the property is supported using a conditional block. Error:

Error: unable to build kubernetes objects from release manifest: error validating "": error validating data: [ValidationError(Service.spec.ports[0]): unknown field "appProcotol" in io.k8s.api.core.v1.ServicePort, ValidationError(Service.spec.ports[1]): unknown field "appProcotol" in io.k8s.api.core.v1.ServicePort]
antonioberben commented 4 months ago

hi @pavelnikolov , the code now makes appProtocol as conditional when:

{{- if and (ge .Capabilities.KubeVersion.Major "1") (ge .Capabilities.KubeVersion.Minor "20") }}

As you can see here the stable version is kubernetes 1.120. Any idea why the lint/tests are failing?

pavelnikolov commented 4 months ago

@antonioberben I believe that you need to check not the Kubernetes version but rather the available API versions. I haven't checked particularly for the appProtocol but I usually check for some API version instead of K8s version. Was this feature included by default in 1.20? Did you have to enable it with some flag maybe? Reading the docs here to me it seems that your check is correct.

antonioberben commented 4 months ago

Hi @pavelnikolov , It was a typo 😞 . Now it should work. Please, can you review it? thanks!

pavelnikolov commented 4 months ago

@antonioberben I just did a little research and I'd like you to use the semver function that helm provides. This feels future-proof: https://helm.sh/docs/chart_template_guide/function_list/#semantic-version-functions

antonioberben commented 4 months ago

@pavelnikolov done! nice catch

antonioberben commented 4 months ago

Everything ready now. Thanks @pavelnikolov

pavelnikolov commented 4 months ago

Thank you for your contribution!