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

Issue 121: update Postgres chart and identify as "dev" #123

Merged
merged 2 commits into from
May 14, 2024

Conversation

plumdog
Copy link
Contributor

@plumdog plumdog commented Feb 22, 2023

Fixes #121.

Thanks for submitting a PR! Please check the boxes below:

  • I have filled in the "Changes" section below?
  • I have filled in the "How did you test this code" section below?
  • I have bumped the version number in /charts/flagsmith/Chart.yaml in the section version or I'm merging to a release branch

Changes

Update the bundled Postgres chart from chart version 8 in the old "stable" repos to chart version 12 in bitnami's repo.

No effort has been made to preserve data. All of the k8s resources will get replaced. Per the warning at https://docs.flagsmith.com/deployment/kubernetes#provided-database-configuration, recommend storing data in an external database

I reviewed the documentation at https://docs.bitnami.com/kubernetes/infrastructure/postgresql/administration/upgrade/ and determined that it would be silly to try to preserve data.

Further, have renamed to dev-postgres to make it very clear what this is for. There will be some documentation to update to accommodate this, but helm does handle our main use case: we want for users who have already set postgresql.enabled=false to still not have an in-cluster postgres after deploying this change, even though the name of the setting is now devPostgresql.enabled. Helm handles this for us, see https://v2.helm.sh/docs/developing_charts/#tags-and-condition-fields-in-requirements-yaml. So if postgresql.enabled is set to something, use that value. Otherwise use devPostgresql.enabled.

Below is the behaviour users will see based on the values they have set in their values:

postgresql.enabled devPostgresql.enabled PG before? PG after? Notes
[unset] [unset] yes yes ✔️
true [unset] yes yes ✔️
false [unset] no no ✔️ (this is the important one)
[unset] true yes yes ✔️
true true yes yes ✔️
false true no no possibly slightly confusing, but inevitable as the values are asking for contradictory things
[unset] false yes no is a change in behaviour, but only a problem if a user has already set devPostgresql.enabled=false prior to upgrading for some reason, but not set postgresql.enabled=false and does want in-cluster Postgres. So this is actually what we want.
true false yes yes possibly slightly confusing, but inevitable as the values are asking for contradictory things
false false no no ✔️

(PG before: "use the value in the first column if set, otherwise be yes"; PG after: "use the value in the first column if set, otherwise use the value in the second column, otherwise be yes")

So I think this behaves correctly, and the only possible surprises are where postgresql.enabled and devPostgresql.enabled are explicitly contradictory.

How did you test this code?

Deployed to minikube and check the upgrade was clean. Checked that postgresql.enabled/devPostgresql.enabled behaved as expected.

@plumdog plumdog force-pushed the issue-121-update-postgres-chart branch from 645d985 to b8f3bb5 Compare February 22, 2023 10:40
@plumdog plumdog force-pushed the issue-121-update-postgres-chart branch from 8931e45 to 679c1a7 Compare June 20, 2023 12:59
@plumdog
Copy link
Contributor Author

plumdog commented Jun 20, 2023

@matthewelwell this is rebased and passing CI.

@matthewelwell
Copy link
Contributor

@plumdog just reading the description, I think one of the sections is unfinished?

I reviewed the documentation at https://docs.bitnami.com/kubernetes/infrastructure/postgresql/administration/upgrade/ and determined that it

@plumdog
Copy link
Contributor Author

plumdog commented Jun 28, 2023

@matthewelwell fixed.

@matthewelwell
Copy link
Contributor

The next question is how we handle the release notes for this - given that there is a breaking change it's going to be critically important. As far as I can tell, we don't currently have release notes anywhere. Do we think it would be sufficient to add these to docs.flagsmith.com or perhaps we should just be adding them to the releases here: https://github.com/Flagsmith/flagsmith-charts/releases

Where would you expect to consume them @plumdog ?

@plumdog
Copy link
Contributor Author

plumdog commented Jun 28, 2023

To use the Postgres chart as a guide, they have a note about upgrading in the chart readme (see https://artifacthub.io/packages/helm/bitnami/postgresql#upgrading which is also https://github.com/bitnami/charts/tree/main/bitnami/postgresql#upgrading) which links to their docs site (and the Postgres docs too)

That said, I guess the main thing is to decided a place where release notes go, and link to it from lots of places, but ultimately you can't intercept someone who applies an upgrade from 1.x to 2.x without reviewing a diff or looking for release notes, only make it very likely that someone who looks for release notes will find them.

So how about:

More knotty would be any outbound communication to users, many of whom won't be known if they're just using the open source version. And anyway, there's probably very little overlap between customers you know about and customers using the bundled in-cluster Postgres.

@plumdog
Copy link
Contributor Author

plumdog commented Jun 28, 2023

@matthewelwell see Flagsmith/flagsmith#2357 for the docs, and have added a link to where those docs will end up to the chart's readme.

@plumdog plumdog force-pushed the issue-121-update-postgres-chart branch 2 times, most recently from d86f352 to 408aa95 Compare August 2, 2023 10:17
@plumdog plumdog force-pushed the issue-121-update-postgres-chart branch from 408aa95 to 3733c7e Compare August 2, 2023 13:31
@plumdog
Copy link
Contributor Author

plumdog commented Aug 2, 2023

@matthewelwell this is now rebased and passing CI, so I think is reviewable.

I also notice that the link from the docs repo will need updating to reference whatever chart version this ends up becoming.

@matthewelwell matthewelwell requested review from gagantrivedi and removed request for matthewelwell April 16, 2024 15:19
@gagantrivedi gagantrivedi merged commit 05d899f into main May 14, 2024
1 check passed
@gagantrivedi gagantrivedi deleted the issue-121-update-postgres-chart branch May 14, 2024 07:15
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.

Update Postgres
3 participants