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.

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

Conversation

SkeLLLa
Copy link
Contributor

@SkeLLLa SkeLLLa commented Nov 25, 2023

Add possibility to define connection URI instead of separate params.

Closes #28

@rishabhpoddar rishabhpoddar changed the base branch from master to 7.0 November 26, 2023 06:42
appVersion: "3.14.0"
appVersion: "7.0.1"
Copy link
Member

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

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
Member

@rishabhpoddar rishabhpoddar Nov 28, 2023

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
Member

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

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
Member

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!

@rishabhpoddar rishabhpoddar merged commit 7d9b279 into supertokens:7.0 Nov 30, 2023
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.

Allow usage of POSTGRESQL_CONNECTION_URI instead of separate db/password/user in helm chart
3 participants