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

[jaeger] [jaeger-operator] add possibility of overriting the namespace #599

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

Conversation

rzjfr
Copy link

@rzjfr rzjfr commented Sep 10, 2024

What this PR does

Adds possibility to override the namespace of the resources

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format,
will close that issue when PR gets merged)

  • fixes #

Checklist

  • DCO signed
  • Commits are GPG signed
  • Chart Version bumped
  • Title of the PR starts with chart name ([jaeger] or [jaeger-operator])
  • README.md has been updated to match version/contain new values

Signed-off-by: Reza J. Bavaghoush <rzjfr@yahoo.com>
@rzjfr rzjfr changed the title feat: add possibility of overriting the namespace [jaeger] [jaeger-operator] add possibility of overriting the namespace Sep 11, 2024
@rzjfr rzjfr marked this pull request as ready for review September 11, 2024 08:21
@pavelnikolov
Copy link
Contributor

This is the first PR I see that changes both the jaeger and jaeger-operator charts at the same time.

@rzjfr Regarding the jaeger chart upgrade, until now, one could override the namespace using --namespace helm flag when installing the chart. It seems that this is not enough for your use case(s) for some reason 🤔

@rzjfr
Copy link
Author

rzjfr commented Sep 11, 2024

Hi @pavelnikolov thanks a lot for the review,

You are absolutely right, you can use --namespace and adding these changes meant to be backward compatible and not break that. Whoever there are cases which explicitly setting the namespace and/or overriding it could be useful. For example if you want to use the chart as a subchart and you want the jaeger to be deployed in a namespace other than the parent or other sibling charts. These kind of complex use cases are more common if you use a single application in argoCD for example or spinnaker and from what I see public charts mostly accommodate overriding the namespace.

So the idea is the change to be absolutely backward compatible, while adding that possibility if someone needed it.

@pavelnikolov
Copy link
Contributor

This is an interesting use case. I appreciate the backwards compatibility thinking, however, I was wondering if you are installing Jaeger in a namespace different than the (parent chart's) release namespace, then shouldn't you create that namespace as well? Have you tested if that change would actually work?
In addition to that, it seems that the chart version needs to be bumped and other items from the PR checklist taken care of.

rzjfr and others added 2 commits September 13, 2024 09:48
Signed-off-by: Reza J. Bavaghoush <rzjfr@yahoo.com>
Signed-off-by: Reza J. Bavaghoush <rzjfr@users.noreply.github.com>
@rzjfr
Copy link
Author

rzjfr commented Sep 13, 2024

Hi again @pavelnikolov,
Thanks for the quick review and sorry for the delay,
I have tested the charts, not every edge case but it was working fine with couple of quick tests

@pavelnikolov
Copy link
Contributor

@batazor @cpanato this PR LGTM from the jaeger chart from PoV. Do you have some comments related to the operator chart?

Signed-off-by: Reza J. Bavaghoush <rzjfr@yahoo.com>
Signed-off-by: Reza J. Bavaghoush <rzjfr@yahoo.com>
@pavelnikolov
Copy link
Contributor

@yurishkuro wdyt about updating both charts within the same PR? It seems that this isn't a common practice.

@yurishkuro
Copy link
Member

@pavelnikolov I have not been involved in this repo's development, so don't have any strong opinion. There is a general principle of keeping PRs small, from that perspective I would split it into two, one per chart. But as far as any functional impact - I don't know.

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.

3 participants