Skip to content

Comments

fix: Add namespace to templates#138

Merged
Steffen911 merged 2 commits intolangfuse:mainfrom
dbirks:add-namespace-to-templates
Oct 22, 2025
Merged

fix: Add namespace to templates#138
Steffen911 merged 2 commits intolangfuse:mainfrom
dbirks:add-namespace-to-templates

Conversation

@dbirks
Copy link
Contributor

@dbirks dbirks commented Apr 21, 2025

Thanks for this chart! I was running it, and noticed that the namespace I was specifying with -n foo was only taking effect in the subcharts. Adding it to the other templates here to apply everywhere.

Testing

Before

cd charts/langfuse
helm dependency build

The current behavior should add the namespace 39 times:

helm template -n foo -f values.lint.yaml . | grep namespace | wc -l

After

Then after checking out this branch, the same command should show 44:

gh pr checkout 138
helm template -n foo -f values.lint.yaml . | grep namespace | wc -l

Important

Add namespace specification to all templates in the Helm chart for consistent namespace application.

  • Behavior:
    • Adds namespace: {{ $.Release.Namespace }} to metadata in ingress.yaml, nextauth-secret.yaml, and postgresql-secret.yaml.
    • Adds namespace: {{ $.Release.Namespace }} to metadata in serviceaccount.yaml, web/deployment.yaml, and web/hpa.yaml.
    • Adds namespace: {{ $.Release.Namespace }} to metadata in web/scaled-object.yaml, web/service.yaml, and web/vpa.yaml.
    • Adds namespace: {{ $.Release.Namespace }} to metadata in worker/deployment.yaml, worker/hpa.yaml, and worker/scaled-object.yaml.
    • Adds namespace: {{ $.Release.Namespace }} to metadata in worker/vpa.yaml.
  • Testing:
    • Before: helm template -n foo -f values.lint.yaml . | grep namespace | wc -l returns 39.
    • After: helm template -n foo -f values.lint.yaml . | grep namespace | wc -l returns 44.

This description was created by Ellipsis for 577fb62. You can customize this summary. It will automatically update as commits are pushed.

@dbirks dbirks requested a review from Steffen911 as a code owner April 21, 2025 23:04
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Apr 21, 2025
@CLAassistant
Copy link

CLAassistant commented Apr 21, 2025

CLA assistant check
All committers have signed the CLA.

@dbirks dbirks changed the title Add namespace to templates fix: Add namespace to templates Apr 21, 2025
@Steffen911
Copy link
Member

@dbirks I'm not sure if I can follow. Wenn I install the chart into a namespace all resources are created in that respective namespace.
Could you add a few more sentences on why you believe that having this explicitly is necessary and helpful?

@devopstechpro
Copy link

I was able to produce the issue by install the helm chart using terraform helm_release resource. For more info see bug #261

@dbirks
Copy link
Contributor Author

dbirks commented Oct 22, 2025

A very belated response from me here, sorry about that.

Seeing the other issue mentioned here about the Terraform helm_release resource made me realize this probably doesn't affect normal helm installs. I'm using Flux's helm controller which must be the difference.

I think it's not required by any means, but just convention, to put the namespace in all the templated resources the chart makes. We can kinda see that by looking at the subcharts that langfuse pulls in, as they all have it defined.

So yes right it looks like helm install isn't affected, but sounds like Terraform and Flux would be helped by this change.

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 22, 2025
@Steffen911 Steffen911 merged commit 6a53ef5 into langfuse:main Oct 22, 2025
3 checks passed
@Steffen911
Copy link
Member

@dbirks @devopstechpro Thank you both for the additional input and the clarification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants