Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Helm: make controller namespace configurable #602

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

csandanov
Copy link

Right now, alloy controller in Helm always deployed into default namespace, despite other resources it relies on, such as service account, use release namespace. The deployment fails as a result. This PR uses the default namespace from helpers

@CLAassistant
Copy link

CLAassistant commented Apr 18, 2024

CLA assistant check
All committers have signed the CLA.

@tpaschalis
Copy link
Member

tpaschalis commented Apr 19, 2024

Hey there, thank you for opening a PR on the Alloy repo!

Could you explain more about what we're trying to achieve here? Did you have an issue with the Alloy helm chart itself, or the k8s-monitoring-helm chart?

Right now, alloy controller in Helm always deployed into default namespace, despite other resources it relies on

In my use clusters, the chart does respect the value I pass to helm commands eg. helm upgrade --install --namespace=alloy ...... Was that not the case for you?

@csandanov
Copy link
Author

csandanov commented Apr 19, 2024

You're right, it actually works to my surprise.

I'm using helm but not Helm CLI and turns out Helm has some very obscure way to override namespace (via kube client config) during helm install/upgrade (but not helm template) even when they're not set (ref helm/helm#3553).

I'm playing with https://github.com/grafana/k8s-monitoring-helm and I just noticed that there are more resources in alloy's helm that do not have namespace but because k8s-monitoring does not use it I didn't notice, worth noting k8s-monitoring's templates do have metadata.namespace in their templates.

I still think it's better to specify namespace explicitly in the templates.

@rfratto rfratto added the backport-to-agent:no PR should NOT be backported to the agent repo. label Apr 23, 2024
Copy link
Contributor

This PR has not had any activity in the past 30 days, so the needs-attention label has been added to it.
If you do not have enough time to follow up on this PR or you think it's no longer relevant, consider closing it.
The needs-attention label signals to maintainers that something has fallen through the cracks. No action is needed by you; your PR will be kept open and you do not have to respond to this comment. The label will be removed the next time this job runs if there is new activity.
Thank you for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-agent:no PR should NOT be backported to the agent repo. needs-attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants