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

Preserve Helm Parameters case #2356

Closed
bryopsida opened this issue Aug 28, 2023 · 3 comments · Fixed by #2369
Closed

Preserve Helm Parameters case #2356

bryopsida opened this issue Aug 28, 2023 · 3 comments · Fixed by #2369
Assignees
Labels
component/helm ⎈ enhancement New feature or request
Milestone

Comments

@bryopsida
Copy link
Contributor

bryopsida commented Aug 28, 2023

Component

JKube Kit

Is your enhancement related to a problem? Please describe

As a user I would like to be able to set/override default values of included helm dependencies as defaults in my generated Values.yaml. This can be problematic when that sub chart uses camelCasing. For example when running k8sResource k8sHelm with the gradle plugin, postgres.someValue ends up as postgres.somevalue in the generated Values.yaml file. This is especially problematic because helm best practices suggest camelCasing: https://helm.sh/docs/chart_best_practices/values/#naming-conventions which means most values have this issue.

Describe the solution you'd like

I would like to see the original case provided in Helm.Parameters to be preserved in the generated Values.yaml. To not break anyone who might already have helm parameters with mixed case currently this would be an opt in toggle to preserve case.

Describe alternatives you've considered

#2200 could solve part of the problem as well as long as the case is preserved from the provided Values.yaml file.

Additional context

No response

@bryopsida bryopsida added the enhancement New feature or request label Aug 28, 2023
@rohanKanojia
Copy link
Member

@bryopsida : I see you've got a PR related to helm open. Do you plan to work on this as well? Shall I assign it to you?

@bryopsida
Copy link
Contributor Author

@rohanKanojia yes, I was planning on taking this on as well.

@manusa
Copy link
Member

manusa commented Sep 22, 2023

I can't really find a reason as to why we're transforming the parameter names to lower-case.

The only reason I can think of is to make them case-insensitive.

The change was added as part of #116 when the Helm logic was extracted from the Mojo to a separate module in JKube Kit.

Fabric8 Maven Plugin's original code doesn't include any case transformation either:

https://github.com/fabric8io/fabric8-maven-plugin/blob/89af88323fa685d8b298ba4b2cbc415b9b2911f0/plugin/src/main/java/io/fabric8/maven/plugin/mojo/build/HelmMojo.java#L682-L684

In conclusion, I think it's safe to preserve the case-sensitivity and I don't really see the need for adding an additional flag. I'll refactor #2369 accordingly.

@manusa manusa added this to the 1.15.0 milestone Sep 22, 2023
@github-project-automation github-project-automation bot moved this from In Progress to Done in Eclipse JKube Sep 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/helm ⎈ enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants