search

surrealdb / helm-charts

4 stars 5 forks source link

migration to 2.0: replace SURREAL_AUTH env var with SURREAL_UNAUTHENTICATED #9

Closed beolnix closed 2 weeks ago

beolnix commented 2 months ago

migration to 2.0: replace SURREAL_AUTH env var with SURREAL_UNAUTHENTICATED while keep support for the old value name in place for backward compatibility and soft deprecation

drunkplato commented 1 month ago

Need to get this merged. Am waiting to upgrade to 2.0.

gguillemas commented 1 month ago

I don't have much experience with Helm, but according to the linter this version needs to be bumped to 0.3.5 in the PR.

EDIT: Probably in this line as well.

beolnix commented 1 month ago

@gguillemas thx, I accepted comment change suggestion and bumped up the version.

I also realised, if I remove SURREAL_AUTH env var, it won't be possible to deploy 1.x with this helm chart, so I put it back. I see these env vars are not in conflict with each other, so I guess it must be fine to have them both and allow surrealdb use one or another depending on the image version 1.x or 2.x

gguillemas commented 1 month ago

The checks are now passing and I agree with keeping SURREAL_AUTH for users still on 1.x. However, I have noticed that the boolean values are provided as a string, so the not operation will not do what you would expect. If you don't mind, I will add a commit to your PR with my proposal to address this change.

gguillemas commented 1 month ago

Hi @beolnix!

I made some changes in 1d47b64 that I believe should provide working soft deprecation while establishing unauthenticated as the standard from now on, including for users working with 1.x deployments. Would you mind having a look and verifying that it would work on the scenarios that you had in mind? We can merge the PR if it looks good to you.

Thank you for helping with this! 🙇

beolnix commented 1 month ago

LGTM, thank you @gguillemas

ericwhitefield commented 1 month ago

I don't have much experience with Helm, but according to the linter this version needs to be bumped to 0.3.5 in the PR.

EDIT: Probably in this line as well.

Good call on updating the version numbers in Readme.md. I think we also should update the appVersion in Chart.yaml.

This also relates to the chart working with both V1 and V2 SurrealDB. Here is the current output of the helm repo.

$ helm search repo surrealdb

NAME                    CHART VERSION   APP VERSION     DESCRIPTION                                       
surrealdb/surrealdb     0.3.4           1.0.0           SurrealDB is the ultimate cloud database for to...

I believe those version values come from Chart.yaml, and the generated template will also fill those values.

One option is that the helm repo can keep older chart versions. For SurrealDB v1 in this case. Which then frees everyone up to change the chart in git for v2, without worrying about backwards compatibility.

It's a minor issue right now. Just the auth setting, but one could imagine breaking changes that are harder to adapt.

It might be a nice idea tag the previous git commit as "chart-v0.3.4". This way the file can be found in git history from the helm repo listing.

mumoshu commented 2 weeks ago

Merging to get the ball rolling. Thank you very much for your contribution @beolnix!

It might be a nice idea tag the previous git commit as "chart-v0.3.4". This way the file can be found in git history from the helm repo listing.

@ericwhitefield Thank you for chiming in! We do have tags like surrealdb-0.3.4 created by chart-releaser. Would it work for your use-case too?

ericwhitefield commented 1 day ago

Thanks @mumoshu you're doing a great job and I appreciate your work on these helm charts.

Yes, my use case is covered. I was making a recommendation to help you and others manage the helm chart updates.

Let me explain.

This is what was published before the recent update:

 $ helm search repo surrealdb

 NAME                    CHART VERSION   APP VERSION     DESCRIPTION                                       
 surrealdb/surrealdb     0.3.4           1.0.0           SurrealDB is the ultimate cloud database for to...

Here is how it looks now after the update:

 $ helm search repo surrealdb

 NAME                    CHART VERSION   APP VERSION     DESCRIPTION                                       
 surrealdb/surrealdb     0.3.6           1.0.0           SurrealDB is the ultimate cloud database for to...

The following is what I was recommending to have after the update (for example) :

 $ helm search repo surrealdb --versions

 NAME                    CHART VERSION   APP VERSION     DESCRIPTION                                       
 surrealdb/surrealdb     0.3.6           2.1.2           SurrealDB is the ultimate cloud database for to...
 surrealdb/surrealdb     0.3.4           1.0.0           SurrealDB is the ultimate cloud database for to...

In this case the "App Version" (SurrealDB Version) has been updated. When people pull the chart it can install that SurrealDB version without modification by hand.

People can still access the old charts for older versions. So backwards compatibility is not necessary.

You did a nice job on the backwards compatibility though. I'm not complaining. I just think you may hit a point in the future where backwards compatibility becomes too difficult. So maybe what I'm showing here will be of use at that time.