fix: add namespace for applicable helm chart resources

[rasmusdencker]

What this PR does

Adds namespace to all Helm resources.

Note: I also removed parts with with .namespace to normalize namespace declarations.

{{- with .namespace }}
  namespace: {{ . }}
  {{- end }}

This is probably considered a breaking change, so let me know if I should reintroduce them and fall back to .Release.Namespace instead. It's noteworthy that this convention was also very inconsistent, so I'm finding it hard to believe that it's used 'in the wild'.

Which issue(s) this PR fixes or relates to

Fixes #2045

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[krajorama]

Hi , thank you for the PR,

What problem does adding the namespace solve? We used to have some namespace definitions, but then we removed them because they are redundant, implied by helm -n anyway.

Also, the ServiceMonitoring needs the current setup. Please see my comment on that inline.

regards, krajo

[krajorama]

Actually here we may want to place this custom resource into the namespace of prometheus operator , and not into the release namespace, hence we used .Values.serviceMonitor.namespace, instead of the Release namespace. I actually have it running with my Mimir being in dev and Prom monitoring in the monitoring namespace:

serviceMonitor:
  enabled: true
  namespace: monitoring
  namespaceSelector:
    matchNames:
      - dev

[rasmusdencker]

Yup, that does make sense. I'll fix this in my next iteration.

[dimitarvdimitrov]

it might still make sense to default to the .Release.Namespace

[dimitarvdimitrov]

Hey @krajorama, there was some prior discussion in the community slack: thread. And I created an issue #2045 to give slightly more context. Basically, not having an explicit namespace makes applying via helm template --namespace foo ... | kubectl apply -f - possible; similar use-case is GitOps. Without explicit namespace resources are created in the current context namespace.

This is probably considered a breaking change

@rasmusdencker not sure if we should worry about this. It seems like the behaviour was mostly consistent, but I don't think it was consistent enough to work without additional tweaks. And I don't think these tweaks will be broken by this change.

[dimitarvdimitrov]

I think you need to rebase this PR to make the CI pass. A fix was merged in #2051

[rasmusdencker]

Got it @dimitarvdimitrov - I'll rebase it in my next iteration.

[krajorama]

Sorry for the long delay, was busy with other things. I see your point. I'll re-review when rebased.

Note: we now have more people working on the chart, so there are rapid changes going on. In particular I think you'll need to run make check-helm-tests after rebase to update the generated manifests we store now in the repo.

[krajorama]

Hi,

I've got an updated version of this fix going in #2123 . @rasmusdencker indicated as co-author.

[rasmusdencker]

Oh, thanks @krajorama. I'll close this one. Sorry for not following up earlier.