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

charts: Replace hardcoded namespaces #939

Merged

Conversation

atanasdinov
Copy link
Contributor

What this PR does / why we need it:

Removes the hardcoded namespace values used within the different chart hooks.

Special notes for your reviewer:

There are some resources created in the default namespace across the pre-install-job and post-upgrade-job hooks. Should those be updated as well?

Checklist:

  • squashed commits into logical changes
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@atanasdinov atanasdinov requested a review from a team as a code owner December 19, 2024 16:48
@kkaempf kkaempf added the kind/enhancement Categorizes issue or PR as related to a new feature. label Dec 20, 2024
@salasberryfin
Copy link
Contributor

Thanks @atanasdinov, I agree this is a sensible thing to support. Maybe it would be great to also have some kind of test that validates that nothing breaks if a user specifies a custom namespace (just to be 100% sure), did you have the chance to test it yourself? There may be some "hidden" references to rancher-turtles-system as it's been considered "Turtles' namespace" until now, that could eventually beak something.

I think it makes sense to also create pre-install/post-upgrade jobs in the user-specified namespace. This one you're also linking

disables the existing Rancher CAPI feature and should stay the same, though.

@kkaempf kkaempf added this to the January 2025 milestone Jan 3, 2025
@atanasdinov
Copy link
Contributor Author

As far as manual testing goes, I didn't see any problems. I also tried to install the chart in a different namespace and there wasn't an issue (where both the chart namespace, as well as the one defined in the values file are the same).

The respective hook jobs are now also transferred over to the chart namespace and confirmed to be working properly. The only caveat for the pre-install ones is that the namespace has to be created first -- either manually or via --create-namespace during the Helm install process (I assume Rancher does it?).

Let me know what you think.

@alexander-demicev alexander-demicev merged commit c14f37b into rancher:main Jan 9, 2025
9 checks passed
@atanasdinov atanasdinov deleted the fix-chart-namespace-usages branch January 9, 2025 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Categorizes issue or PR as related to a new feature.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants