drain-cleaner
drain-cleaner copied to clipboard
strimzi
Improve Namespace handling in the chart: Set release Namespace as default and allow user overrides
In the current situation, the creation of the namespace is managed by the 'create' configuration as we can see here: https://github.com/strimzi/drain-cleaner/blob/main/helm-charts/helm3/strimzi-drain-cleaner/templates/000-Namespace.yaml#L1.
On the other hand, the deployment is governed by the 'name' configuration at this location: https://github.com/strimzi/drain-cleaner/blob/main/helm-charts/helm3/strimzi-drain-cleaner/templates/060-Deployment.yaml#L7.
This leads to unusual behavior when the 'create' configuration is set to false, but the HelmRelease still attempts to deploy in the default 'strimzi-drain-cleaner' namespace.
Using the default namespace in this manner may not be the best practice, as typically, resources deployed by a chart share the same namespace as the release by default.
A suggested solution would be to adjust the conditions and set the release namespace as the default. This should only be overridden if the user specifically requests it.
What do you think?
The default namespace is not a place you want to deploy applications. It usually has special properties and might not even allow applications to run properly.
TBH, I think it works well from my point of view today:
- If you disable the namespace creation, then you are ultimately saying that you are responsible for creating the namespace. so you should not complain if it fails because the namespace does not exist.
- Changing the namespace used as default today to something else would mean backwards incompatible change. So I think we should stick with 'strimzi-drain-cleaner'.
@scholzj So we just ran into a bug where the drain cleaner Certificate is invalid and not being creaetd properly - and we didn't notice. The issue is in https://github.com/strimzi/drain-cleaner/blob/0.4.2/helm-charts/helm3/strimzi-drain-cleaner/templates/041-Certificate.yaml#L13-L15. Why are you specifying .Values.namespace.name rather than using the fairly standard {{ .Release.Namespace }} or even {{ default .Release.Namespace .Values.namespace.name }}? I find it generally unusual to see helm charts where I have to hard-code in the namespace name... and that makes using tools like ct more difficult.
