keephq/helm-charts

[BUG]: Helm chart missing websocket ingress template

jackrh opened this issue · 5 comments

Description of the bug

If you add an ingress to websocket in your values.yaml it will never render the necessary yaml because the websocket template file is missing.

Steps To Reproduce

  1. Add ingress to values.yaml file for websocket
websocket:
  enabled: true
  ingress:
    enabled: true
    hosts:
      - host: keep-websocket.${SECRET_DOMAIN}
        paths:
          - path: /
            pathType: Prefix
    tls:
      - hosts:
          - keep-websocket.${SECRET_DOMAIN}

  1. Install helm chart
  2. Verify no ingress is created

Additional Information

No response

I don't think websocket service ever need to have ingress object at all. Instead of adding template for it, removing ingress part from values.yaml would be better. @talboren @shahargl what do you guys think?

I don't think websocket service ever need to have ingress object at all. Instead of adding template for it, removing ingress part from values.yaml would be better. @talboren @shahargl what do you guys think?

Hmm, but we do need ingress for the websocket service if we want FE clients to be able to get push notifications about newly added alerts (real-time updates) -- lmk if I miss understood something

I don't think websocket service ever need to have ingress object at all. Instead of adding template for it, removing ingress part from values.yaml would be better. @talboren @shahargl what do you guys think?

Hmm, but we do need ingress for the websocket service if we want FE clients to be able to get push notifications about newly added alerts (real-time updates) -- lmk if I miss understood something

Actually you don't need ingress for that if you are not going to access your websocket service from another cluster. If both websocket and fe services are in same cluster it is kind of best practice to communicate them over services. like: keep-websocket.<namespace>.svc.cluster.local:6001

Contributor

gotcha! thanks for the detailed explanation! @pehlicd do you want to craft a PR to issue that?

yeah sure