search

langfuse / langfuse-k8s

Community-maintained Kubernetes config and Helm chart for Langfuse
https://langfuse.com
MIT License
44 stars 24 forks source link

feat: support extra containers and k8s manifests #10

Closed vinivasundharan closed 1 month ago

CLAassistant commented 4 months ago

CLA assistant check
All committers have signed the CLA.

marcklingen commented 3 months ago

Hi, can you expand on why you think this change is necessary or helpful?

vinivasundharan commented 3 months ago

Hi. we use langfuse and we use cloudsql proxy for connecting our db, and extra secret to hold the JSON for the SA.

frittentheke commented 2 months ago

@marcklingen this is really very common (https://github.com/search?q=extraVolumes+extraVolumeMounts+helm&type=code) to expose such injection points in Helm Charts via the values.yaml to allow for required local customization that you cannot possible foresee as chart maintainer. Usually they are extraEnv, extraVolumes and extraVolumeMounts, see NGINX Ingress Helm chart as an example: https://github.com/kubernetes/ingress-nginx/blob/4b5c5efe2508dc915a48c54de7f23912ff2ec695/charts/ingress-nginx/README.md?plain=1#L313-L317 or the template Bitnami publishes for adding to charts to their huge library of charts: https://github.com/bitnami/charts/blob/70ca413aee4bc937aa1bc530c11261ef75bab800/template/README.md?plain=1#L34

@vinivasundharan

  1. You seem to miss extraVolumeMounts in your PR, extra volumes don't help if you can't mount them ;-)
  2. You might want to align all those additions with the common prefix extra, so to have: extraEnv,extraVolumes, extraVolumeMounts, extraContainers and extraInitContainers (never know if Langfuse itself might come with an init-container)
vinivasundharan commented 2 months ago

@marcklingen this is really very common (https://github.com/search?q=extraVolumes+extraVolumeMounts+helm&type=code) to have in Helm charts expose such injection points via their values.yaml to allow for required local customization that you cannot possible foresee. Usually they are extraEnv, extraVolumes and extraVolumeMounts, see NGINX Ingress Helm chart as an example: https://github.com/kubernetes/ingress-nginx/blob/4b5c5efe2508dc915a48c54de7f23912ff2ec695/charts/ingress-nginx/README.md?plain=1#L313-L317 or the template Bitnami publishes for adding to charts to their huge library of charts: https://github.com/bitnami/charts/blob/70ca413aee4bc937aa1bc530c11261ef75bab800/template/README.md?plain=1#L34

@vinivasundharan

  1. You seem to miss extraVolumeMounts in your PR, extra volumes don't help if you can't mount them ;-)
  2. You might want to align all those additions with the common prefix extra, so to have: extraEnv,extraVolumes, extraVolumeMounts, extraContainers and extraInitContainers (never know if Langfuse itself might come with an init-container)

@frittentheke Thanks for pointing out. Renaming the variables. I have added mounting of extra Volumes to the main container as well. I did not want to mount extraVolumes to the main container itself, but maybe someone else needs it.

mautini commented 1 month ago

It seems a great addition for me. It makes the helm chart more versatile. We just need to update the helm chart version before merging (bumping minor version according semver)