-
Notifications
You must be signed in to change notification settings - Fork 59
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
[fix] Update chart to handle the opt-in mechanism for Java dbconnecto… #190
Conversation
…r before 3.93-edge
- name: DISABLE_JAVA_DBCONNECTOR | ||
value: "true" | ||
{{ end }} | ||
{{ if and ( or (eq "latest" $.Values.image.tag ) ( semverCompare ">= 3.93.0-0" $.Values.image.tag ) ) ( .Values.jobRunner.enabled ) }} |
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 semverCompare
might give you trouble with image tags that are like 3.xx.x-stable
or -edge
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 tested against the -edge
ones and it works fine. I think it supports them because it's supposed to support SHAs there. That's why the comparison is against 3.93.0-0
.
I might add extra tests against a helm chart that has those versions just in case!
- name: SERVICE_TYPE | ||
value: MAIN_BACKEND,DB_CONNECTOR,DB_SSH_CONNECTOR,JOBS_RUNNER | ||
{{ end }} | ||
{{ if and ( or (eq "latest" $.Values.image.tag ) ( semverCompare ">= 3.93.0-0" $.Values.image.tag ) ) ( not $.Values.dbconnector.java.enabled ) }} |
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.
the long if list feels like trouble, esp if we want to change how we construct these in the future (which we'll certainly do, esp with splitting out main backend and dbc)..i wonder if we could do something like
`value: MAIN_BACKEND{{ if ",DB_CONNECTOR" else "" }}{{if ",JAVA_DBCONNECTOR" else "" }}
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.
maybe, that's a good idea let me try it
The reason for this is that the backend needs to act as a jobs runner, if the jobs runner is not enabled. | ||
*/}} | ||
{{- if not .Values.jobRunner.enabled }} | ||
{{- $serviceType = append $serviceType "JOBS_RUNNER" }} |
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.
wait, the workflow backend should never have the jobs runner service. it wasn't in the original list either
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.
whoops
{{- if and ( not $retool_version_with_java_dbconnector_opt_out ) ( $.Values.dbconnector.java.enabled ) }} | ||
{{- $serviceType = append $serviceType "JAVA_DBCONNECTOR" }} | ||
{{- end }} | ||
{{- /* |
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.
love the comments!
…r before 3.93-edge
In chart version 6.2.9, we changed Java dbconnector to be opt out, using a new system that uses an env var so that it's opt out even outside of Helm. This PR adds back in the original functionality for Java dbconnector opt-in logic (although now it will be opt-out due to the change in the values file default), so that customers on older versions of Retool won't see regressions as they upgrade their helm chart version.