-
Notifications
You must be signed in to change notification settings - Fork 130
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
Helm chart: add support for MariaDB and MySQL backends #8554
Conversation
0151e5c
to
fd1e0a1
Compare
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.
LGTM 👍
@@ -139,20 +139,31 @@ spec: | |||
{{- end }} | |||
|
|||
{{- if or (eq .Values.versionStoreType "JDBC") (eq .Values.versionStoreType "TRANSACTIONAL") }} | |||
- name: QUARKUS_DATASOURCE_USERNAME | |||
{{- $oldConfig := .Values.postgres | default dict }} |
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.
I trust you and @dimas-b on the helm/go magic happening here :)
postgres: | ||
# -- The Postgres JDBC connection string. | ||
# JDBC datasource settings. Only required when using JDBC version store type; ignored otherwise. | ||
jdbc: |
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.
This looks like a breaking change for existing users?
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.
Same for the secrets-stuff below
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.
It isn't: the old postgres
section is still honored thanks to the "helm magic" ;-)
IOW you can do this:
helm install nessie -n nessie-ns helm/nessie --debug --dry-run \
--set versionStoreType=JDBC \
--set postgres.jdbcUrl=jdbc:postgresql://foo/bar \
--set postgres.secret.password=old-password-key
And it will merge the old postgres.xyz
values with the new defaults coming from jdbc.xyz
:
- name: NESSIE_VERSION_STORE_PERSIST_JDBC_DATASOURCE
value: postgresql
- name: QUARKUS_DATASOURCE_POSTGRESQL_USERNAME
valueFrom:
secretKeyRef:
name: datasource-creds
key: username
- name: QUARKUS_DATASOURCE_POSTGRESQL_PASSWORD
valueFrom:
secretKeyRef:
name: datasource-creds
key: old-password-key
- name: QUARKUS_DATASOURCE_POSTGRESQL_JDBC_URL
value: jdbc:postgresql://foo/bar
2. Migrate any property under `quarkus.datasource.*` to `quarkus.datasource.postgresql.*`. Support | ||
for the old `quarkus.datasource.*` properties will be removed in a future release. | ||
- For the same reason, the Nessie Helm chart has been updated. The old `postgres` section is now | ||
called `jdbc`. Existing Helm chart configurations should be updated accordingly, e.g. |
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.
I think this needs to be added to Breaking changes
below as well
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.
Do we still want this under the "Breaking Changes" section? From my POV it's not required since we still support old properties through "helm magic" :)
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.
As long as the old ones work (without Helm & its magic), that's fine for me.
edf8129
to
dbbba89
Compare
Requires #8544.