-
Notifications
You must be signed in to change notification settings - Fork 46
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
feat(helm): add connection uri param #29
feat(helm): add connection uri param #29
Conversation
* Bump image tag * Bump app ver
helm-chart/Chart.yaml
Outdated
appVersion: "3.14.0" | ||
appVersion: "7.0.1" |
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.
Could you please explain why this version bump and not 3.15.0?
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 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.
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.
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?
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 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:
- You update something in supertokens-core and publish new version
x.y.z
- New docker image is built with tag
x.y.z
- In helm chart only
appVersion
is updated tox.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.
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.
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
inhelm-chart/values.yaml
was also changed to7.0.1
. What doestag
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 thelatest
image? If so, maybe we should set the image tag to7.0
as well, since auto pullinglatest
is a bad idea (since it can auto pull in breaking changes)
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 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.
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.
Yea, that's odd. The tag should be changed to 7.0
. Which i see that you have done. Thanks!
* Bump Versions (supertokens#27) * Bump image tag * Bump app ver * feat(helm): add connection uri param * fix: versions --------- Co-authored-by: Aayushman Choudhary <[email protected]>
Add possibility to define connection URI instead of separate params.
Closes #28