[jaeger] fix: missing query.rules.paths.path, default to /

jaegertracing / helm-charts

Helm Charts for Jaeger backend

Apache License 2.0

265 stars 340 forks source link

[jaeger] fix: missing query.rules.paths.path, default to / #420

Closed steache closed 1 year ago

steache commented 1 year ago

What this PR does

fixes query.ingress for nginx-ingress-controller (probably more ingress controllers)
used in here https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/templates/query-ing.yaml#L4 and here https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/templates/query-ing.yaml#L28 but not defined :)
if it's not defined, chart will produce paths like this:

- pathType: ImplementationSpecific
  backend:
    service:
      name: jaeger-query
      port:
        number: 80

missing the path: /, I'm not sure if it's problem only for nginx-ingress-controller or for more controllers

Checklist

[x] DCO signed
[x] Commits are GPG signed
[x] Chart Version bumped
[x] Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
[x] README.md has been updated to match version/contain new values

mehta-ankit commented 1 year ago

@steache just for my curiosity, what's the problem if path: / is not included 🤔. Also collector ingress has the same design: https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/templates/collector-ing.yaml#L28

steache commented 1 year ago

@steache just for my curiosity, what's the problem if path: / is not included 🤔. Also collector ingress has the same design: https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/templates/collector-ing.yaml#L28

@mehta-ankit

good question, sorry for not mentioning it in the first place
we are using bitnami/nginx-ingress-controller@9.3.23 and it's failing to pick up jaeger's ingress due to "misconfigured" path
after changing it to path: / it started to work :)
I have to admit, that I'm not sure if it's something that changed in jaeger or maybe in new release of nginx

edit: oh I see! I'll add default value for the collector as well

mehta-ankit commented 1 year ago

@steache just for my curiosity, what's the problem if path: / is not included 🤔. Also collector ingress has the same design: https://github.com/jaegertracing/helm-charts/blob/main/charts/jaeger/templates/collector-ing.yaml#L28

@mehta-ankit

good question, sorry for not mentioning it in the first place

we are using bitnami/nginx-ingress-controller@9.3.23 and it's failing to pick up jaeger's ingress due to "misconfigured" path

after changing it to path: / it started to work :)

I have to admit, that I'm not sure if it's something that changed in jaeger or maybe in new release of nginx

edit: oh I see! I'll add default value for the collector as well

Thanks @steache . Also i see the base path is used to create an env var for query and collector deployments: https://github.com/jaegertracing/helm-charts/pull/420#issuecomment-1342831266

Wanted to share to make sure its not messing up anything 😄 . Thanks for the change though!

mehta-ankit commented 1 year ago

@steache can you please GPG sign your commit.

steache commented 1 year ago

@steache can you please GPG sign your commit.

@mehta-ankit signed, sorry! :)

mehta-ankit commented 1 year ago

@steache can you fix the linting issue on the PR and maybe please squash your commits. These individual commits and their changes are not making much sense tbh.

steache commented 1 year ago

sorry for trying again in https://github.com/jaegertracing/helm-charts/pull/422, I was not able to rearrange it properly