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.

Sign up for GitHub

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

feat(helm): add connection uri param #29

Merged
merged 3 commits into from
Nov 30, 2023
Merged

feat(helm): add connection uri param #29

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
952b72b
Bump Versions (#27)
xatuke Oct 9, 2023
83b8cd4
feat(helm): add connection uri param
SkeLLLa Nov 25, 2023
577ebf1
fix: versions
SkeLLLa Nov 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions helm-chart/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,10 @@ type: application
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.2
version: 0.1.3

# This is the version number of the application being deployed. This version number should be
# incremented each time you make changes to the application. Versions are not expected to
# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.
appVersion: "3.14.0"
appVersion: "7.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain why this version bump and not 3.15.0?

Copy link
Contributor Author

@SkeLLLa SkeLLLa Nov 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The general idea of it is to indicate what application (supertokens-core) version is bundled, e.g. to be one of versions from https://github.com/supertokens/supertokens-core/releases. I guess I've just mistyped and it should've be 7.0.13.

If it reflects some other version in your case, let me know, since I was unable to find any version that looks like 3.x.x neither in this repo, nor in others.

Copy link
Contributor

@rishabhpoddar rishabhpoddar Nov 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So one issue with this is that the helm chart could support more than one version of the core since there are core changes that don't affect any of the configs provided to it. In that case, updating the helm chart version is unnecessary. For example, right now, we deployed core version 7.0.14, which didn't have any changes related to the helm chart.

What do you think is the right solution in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The helm chart has two "versions", the version of chart itself and the version of application.
The general rule is:

  • chart version is updated whenever there are changes in some chart (e.g. like this one with adding new variable)
  • chart appVersion is changed to indicate what version of app is bundled.
    (The comments in helm chart itself describe it in details)

Ideally how the flow should look like:

  1. You update something in supertokens-core and publish new version x.y.z
  2. New docker image is built with tag x.y.z
  3. In helm chart only appVersion is updated to x.y.z. and in values the proper docker image tag is set.

However in this case if you have for example docker image with tag 7.0 which will contain all patch fixes, we can just put something like 7.0.0 into appVersion in order to show users which "major" version is bundled in this chart.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha! We are planning on making the docker tag x.y.z as well, but until then, what you said about making it 7.0.0 makes sense. Two more questions:

  • I noticed the tag in helm-chart/values.yaml was also changed to 7.0.1. What does tag mean?
  • Setting the appVersion seems to have nothing to do with the actual docker image being pulled in right? So in this case, it will always pull in the latest image? If so, maybe we should set the image tag to 7.0 as well, since auto pulling latest is a bad idea (since it can auto pull in breaking changes)

Copy link
Contributor Author

@SkeLLLa SkeLLLa Nov 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The tag was changed in #27. https://github.com/supertokens/supertokens-docker-postgresql/pull/27/files#diff-15210b6a69967e9488632bf12df699fc922b886f3b38d79fdef46502d8bdbd30R5

And tag is just a variable, that's later used in Deployment and basically represents which docker image tag will be pulled.

BTW, this question also related to #30, since now actually there's no 7.0.1 tag in dockerhub registry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, that's odd. The tag should be changed to 7.0. Which i see that you have done. Thanks!

1 change: 1 addition & 0 deletions helm-chart/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ SuperTokens Core Deployment
| autoscaling.minReplicas | int | `1` | minimal replica count when autoscaling |
| autoscaling.targetCPUUtilizationPercentage | int | `80` | target CPU usage before autoscaling |
| basePath | string | `""` | |
| database.uri | string | `""` | database uri (connection string), used instead of other DB params if not empty |
| database.host | string | `"postgres.postgres"` | database host |
| database.name | string | `"supertokens"` | database name |
| database.password | string | `"password"` | database password |
Expand Down
4 changes: 4 additions & 0 deletions helm-chart/templates/secrets.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,15 @@ metadata:
{{ include "supertokens.labels" . | indent 4}}
type: Opaque
stringData:
{{- if .Values.database.uri }}
POSTGRESQL_CONNECTION_URI: {{ quote .Values.database.uri }}
{{- else }}
POSTGRESQL_DATABASE_NAME: {{ quote .Values.database.name }}
POSTGRESQL_PORT: {{ quote .Values.database.port }}
POSTGRESQL_HOST: {{ quote .Values.database.host }}
POSTGRESQL_USER: {{ quote .Values.database.user }}
POSTGRESQL_PASSWORD: {{ quote .Values.database.password }}
{{- end }}
{{- if .Values.apiKeys.enabled }}
API_KEYS: "{{ join "," .Values.apiKeys.keys }}"
{{- end }}
Expand Down
4 changes: 3 additions & 1 deletion helm-chart/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ image:
repository: supertokens/supertokens-postgresql
pullPolicy: IfNotPresent
# -- Overrides the image tag whose default is the chart appVersion.
tag: "3.14"
tag: "7.0.1"

replicaCount: 1
imagePullSecrets: []
Expand All @@ -13,6 +13,8 @@ fullnameOverride: ""
existingSecret: null

database:
# -- database uri (connection string), used instaed of other params if not empty
uri: ""
# -- database name
name: supertokens
# -- database port
Expand Down