Skip to content

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

Merged
merged 10 commits into from
Jun 30, 2025

Conversation

jparkzz
Copy link
Contributor

@jparkzz jparkzz commented Jun 25, 2025

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

  • Add relational-jdbc persistence type support to Helm chart
  • Use relational-jdbc as the default persistence type
  • Inject JDBC configuration (username, password, jdbc_url) through Kubernetes Secrets as environment variables
  • Maintain backward compatibility with eclipse-link

Additional Details

  • Updated persistence-values.yaml for CI testing
  • Updated test coverage for relational-jdbc configuration
  • JDBC credentials are injected via QUARKUS_DATASOURCE_* environment variables from Secret
  • Secret keys: username, password, jdbc_url

@@ -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 -}}
Copy link
Contributor

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Cf. #1945

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor

@dimas-b dimas-b left a 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 :)

jparkzz and others added 5 commits June 27, 2025 09:39
Co-authored-by: Alexandre Dutra <adutra@users.noreply.github.com>
Co-authored-by: Alexandre Dutra <adutra@users.noreply.github.com>
@jparkzz jparkzz requested a review from adutra June 27, 2025 05:41
adutra
adutra previously approved these changes Jun 27, 2025
@github-project-automation github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board Jun 27, 2025
@jparkzz
Copy link
Contributor Author

jparkzz commented Jun 27, 2025

@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.

@jparkzz jparkzz requested a review from adutra June 27, 2025 08:41
@dimas-b
Copy link
Contributor

dimas-b commented Jun 27, 2025

In Quarkus, when injecting configuration options as environment variables, they need to be in uppercase with underscores. [...]

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. quarkus.datasource.jdbc.url).

Uppercasing is usually necessary in shells (like bash) but k8s is more lenient and allows names like quarkus.datasource.jdbc.url, etc.

@jparkzz
Copy link
Contributor Author

jparkzz commented Jun 28, 2025

In Quarkus, when injecting configuration options as environment variables, they need to be in uppercase with underscores. [...]

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. quarkus.datasource.jdbc.url).

Uppercasing is usually necessary in shells (like bash) but k8s is more lenient and allows names like quarkus.datasource.jdbc.url, etc.

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!

@jparkzz jparkzz requested a review from dimas-b June 28, 2025 12:57
@singhpk234 singhpk234 merged commit d410e9c into apache:main Jun 30, 2025
11 checks passed
@github-project-automation github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board Jun 30, 2025
@singhpk234
Copy link
Contributor

singhpk234 commented Jun 30, 2025

Thank you for the contribution @jparkzz ! Welcome to the Polaris community !

Thanks a ton for reviews @adutra @dimas-b !

@dimas-b
Copy link
Contributor

dimas-b commented Jun 30, 2025

@singhpk234 : this comments was not really addressed : #1937 (comment) it would have been nice to allow some more review time for before merging :)

@jparkzz jparkzz deleted the feat/add-relation-jdbc-to-helm branch June 30, 2025 22:10
@singhpk234
Copy link
Contributor

@dimas-b noted ! Apologies, I missed your comment there, I saw @adutra's approval and I was also +1 so i merged.
I agree with you here that db-kind should configured at build time. Please let me know how can i help, happy to put out a pr for this !

@adutra
Copy link
Contributor

adutra commented Jul 1, 2025

@dimas-b noted ! Apologies, I missed your comment there, I saw @adutra's approval and I was also +1 so i merged.

I agree with you here that db-kind should configured at build time. Please let me know how can i help, happy to put out a pr for this !

Apologies, I approved too quickly 😅 Indeed db-kind cannot be modified at runtime so we should remove it from the Helm chart.

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