-
Notifications
You must be signed in to change notification settings - Fork 269
Add relational-jdbc to helm #1937
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 relational-jdbc to helm #1937
Conversation
@@ -53,6 +53,11 @@ data: | |||
{{- $_ = set $map "polaris.persistence.eclipselink.persistence-unit" .Values.persistence.eclipseLink.persistenceUnit -}} | |||
{{- $_ = set $map "polaris.persistence.eclipselink.configuration-file" (printf "%s/persistence.xml" .Values.image.configDir ) -}} | |||
{{- end -}} | |||
{{- if eq .Values.persistence.type "relational-jdbc" -}} | |||
{{- if .Values.persistence.relationalJdbc.dbKind -}} | |||
{{- $_ = set $map "quarkus.datasource.db-kind" .Values.persistence.relationalJdbc.dbKind -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
quarkus.datasource.db-kind
is a build-time setting. It is probably not useful to set it in helm.
https://quarkus.io/guides/datasource#quarkus-datasource_quarkus-datasource-db-kind
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cf. #1945
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jparkzz : I still believe this configuration setting is not necessary and could be misleading because the value is not really changeable at the helm level. Would you mind following you with another PR to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer to configure dbKind within the secret? With current chart, even if we set it in the secret, I believe the value would still be explicitly visible in the helm chart when it's injected as an environment variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, my point is that db-kind
can only be configured at build time (link above). It is not possible to overwrite that in Helm.
ATM only PostgreSQL is supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jparkzz : Thanks for you contribution! It is very timely :)
The LGTM, but I'll leave final reviews to people more experienced in Helm :)
Co-authored-by: Alexandre Dutra <adutra@users.noreply.github.com>
Co-authored-by: Alexandre Dutra <adutra@users.noreply.github.com>
@adutra In Quarkus, when injecting configuration options as environment variables, they need to be in uppercase with underscores. For secret keys, camelCase can be used as well. The test was failing due to this issue, so I made the necessary corrections. |
This is not exactly correct. AFAIK, Quarkus understands env. variables with mixed case and special chars in the name as long as they match canonical configuration property names (e.g. Uppercasing is usually necessary in shells (like |
You were right. I verified that even if I use the original property name in lowercase with dot as an environment variable in quarkus, the property is loaded just fine. Thanks for the advice! |
@singhpk234 : this comments was not really addressed : #1937 (comment) it would have been nice to allow some more review time for before merging :) |
Apologies, I approved too quickly 😅 Indeed db-kind cannot be modified at runtime so we should remove it from the Helm chart. |
Motivation for the Change
Polaris needs to support relational-jdbc as the default persistence type for simpler database configuration and better cloud-native deployment experience.
Description of the Status Quo (Current Behavior)
Currently, the Helm chart only supports eclipse-link persistence type as the default, which requires complex JPA configuration with persistence.xml files.
Desired Behavior
Additional Details