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

feat: Add support for Helm namespaceOverride #12321

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jgmartinez
Copy link

@jgmartinez jgmartinez commented Mar 22, 2024

What this PR does / why we need it:

This PR introduces support for overriding Helm namespace in multi-chart deployments (via Dependencies). With the new value namespaceOverride users can now define arbitrary namespaces, independent from the main release namespace.

Checklist

  • Reviewed the CONTRIBUTING.md guide (required)
  • Documentation added
  • CHANGELOG.md updated
    • If the change is worth mentioning in the release notes, add add-to-release-notes label
  • For Helm chart changes bump the Helm chart version in production/helm/loki/Chart.yaml and update production/helm/loki/CHANGELOG.md and production/helm/loki/README.md. Example PR

@jgmartinez jgmartinez requested a review from a team as a code owner March 22, 2024 14:12
@CLAassistant
Copy link

CLAassistant commented Mar 22, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/helm type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories labels Mar 22, 2024
Signed-off-by: Juan González <38658722+jgmartinez@users.noreply.github.com>
@@ -2896,7 +2896,7 @@ null
<td><pre lang="json">
{
"name": "self-monitoring",
"secretNamespace": "{{ .Release.Namespace }}"
"secretNamespace": "{{ include "loki.namespace" . }}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you edit this file directly? Or was this change automatically generated? The Helm reference should be updated in production/helm/loki/reference.md.gotmlp. There's a note to that effect at line 11 of the docs/sources/setup/install/helm/reference.md file, you might have missed it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I missed it. Thanks for the heads up :)

@github-actions github-actions bot removed the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Mar 25, 2024
@slim-bean
Copy link
Collaborator

I've been working a lot more with Helm lately but am still learning...

Can you help explain a bit more how this gets used? or why this is needed?

Is this a pattern that a lot of charts follow?

Also as an FYI I've got a branch with a lot of changes we are making to add the distributed version of the chart, this comes with a lot of changes so I think we'll likely have to wait until that is merged before we can merge this (so probably not until later next week) Sorry for the inconvenience here

@JStickler JStickler removed the request for review from MichelHollands April 19, 2024 17:27
@npapapietro
Copy link

I've been working a lot more with Helm lately but am still learning...

Can you help explain a bit more how this gets used? or why this is needed?

Is this a pattern that a lot of charts follow?

Also as an FYI I've got a branch with a lot of changes we are making to add the distributed version of the chart, this comes with a lot of changes so I think we'll likely have to wait until that is merged before we can merge this (so probably not until later next week) Sorry for the inconvenience here

As someone looking forward to this change, one reasons (other than the rest of the grafana charts offer this option, loki's chart seems to break the pattern):

Bundling as a dependency; helm doesn't let you select dependency namespaces independently without it. A common use case for me is having a bootstrapping or baseline set of apps in a single helmchart that will go up in different namespaces of a cluster without needing any tools other than helm

@jgmartinez
Copy link
Author

Hi and sorry for the delay. I'll need to look further into this PR, because chart dependencies also need to support namespace override, and some of them currently don't.

After testing it in our environment there are some things still breaking 😞

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

Successfully merging this pull request may close these issues.

None yet

5 participants