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

add namespace property to all namespaced objects #396

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

rbjorklin
Copy link
Contributor

Using the helm built-in .Release.Namespace as per: https://helm.sh/docs/chart_template_guide/builtin_objects/

@rbjorklin rbjorklin requested review from Ferroin, ilyam8 and a team as code owners November 17, 2023 22:50
@rbjorklin rbjorklin requested review from witalisoft and M4itee and removed request for a team November 17, 2023 22:50
@ilyam8
Copy link
Member

ilyam8 commented Nov 17, 2023

Hey, @rbjorklin. Thanks for the contribution! But can you please explain why this is needed? Did you have a problem installing the current chart? I see that this topic is somewhat controversial and not really a best practice according to one of helm devs. Also, I don't see it is recommended in Helm Best Practices docs (at least I couldn't find it).

@rbjorklin
Copy link
Contributor Author

rbjorklin commented Nov 18, 2023

Hey @ilyam8! Thanks for taking the time to dig up those links.
This might boil down to a limitation in our internal tooling but without these changes the chart does not apply correctly. We are currently using an internal fork of the chart with these changes and I was hoping to upstream them.

EDIT: The proposed change should be okay as .Release.Namespace takes on the value provided to helm template --namespace so the end result is the same.

EDIT2: The ClusterRoleBinding already uses .Release.Namespace so if anything this will just ensure consistency in the deployment.

@ilyam8
Copy link
Member

ilyam8 commented Nov 18, 2023

I think it is ok to change, the only thing I have in mind (not a k8s specialist) that would break is

  • generating with helm (and don't provide --namespace, it will explicitly set the namespace to default)
  • pipe the result to kubectl with --namspace != default

e.g.

helm template . | kubectl apply --namespace someNamespace -f -

but I am not sure why someone would want to do it.

Copy link
Contributor

@M4itee M4itee left a comment

Choose a reason for hiding this comment

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

It basically changes nothing in grand scheme of things in terms of functionality. If it helps some people to avoid some kind of problem/bug - let's go.

@ilyam8 ilyam8 merged commit fa44120 into netdata:master Nov 20, 2023
1 of 2 checks passed
@rbjorklin rbjorklin deleted the add-namespace branch January 10, 2024 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants