lunny/helm-chart
1
0
Fork 0
Code Issues 36 Pull Requests 14 Actions Packages Projects Releases 93 Wiki Activity

Bump postgres chart to latest release #391

Merged
pat-s merged 23 commits from refs/pull/391/head into main 2023-03-27 17:12:30 +00:00
Conversation 2 Commits 23 Files Changed 5 +45 -35
pat-s commented 2023-01-05 09:09:35 +00:00 (Migrated from gitea.com)
Copy Link

See discussion in #387

Upgrade notes to Chart v11.x and Postgres 14.x: https://docs.bitnami.com/kubernetes/infrastructure/postgresql/administration/upgrade/

The current version in Gitea is using 11.11.0-debian-10-r62 from 2021-04.

Bumping the chart to the latest (v12.x) would use the image 15.2.0-debian-11-r14 which would be a jump from postgres 11 to postgres 15. There are no specific notes for the v12.x chart release, hence we might be able to just go to 12.x directly.

There have been some param renamings which I've reflected in the README.

⚠️ BREAKING

Users have to migrate their Postgres DB by e.g. restoring a previously created database dump into a clean installation.

See discussion in #387 Upgrade notes to Chart v11.x and Postgres 14.x: https://docs.bitnami.com/kubernetes/infrastructure/postgresql/administration/upgrade/ The current version in Gitea is using `11.11.0-debian-10-r62` from 2021-04. Bumping the chart to the latest (v12.x) would use the image `15.2.0-debian-11-r14` which would be a jump from postgres 11 to postgres 15. There are no specific notes for the v12.x chart release, hence we might be able to just go to 12.x directly. There have been some param renamings which I've reflected in the README. **⚠️ BREAKING** Users have to migrate their Postgres DB by e.g. restoring a previously created database dump into a clean installation.
techknowlogick (Migrated from gitea.com) reviewed 2023-02-25 10:25:11 +00:00
justusbunsi (Migrated from gitea.com) reviewed 2023-02-25 10:25:12 +00:00
techknowlogick (Migrated from gitea.com) approved these changes 2023-02-26 06:12:20 +00:00
justusbunsi (Migrated from gitea.com) reviewed 2023-02-26 13:07:48 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

Chart.lock must be updated, too. And we should mention that there is a proper way of updating PGSQL databases.

`Chart.lock` must be updated, too. And we should mention that there is a proper way of updating PGSQL databases.
justusbunsi (Migrated from gitea.com) reviewed 2023-02-26 15:14:32 +00:00
README.md
@ -1178,4 +823,3 @@
| `test.image.name` | Image name for the wget container used in the test-connection Pod. | `busybox` |
| `test.image.tag` | Image tag for the wget container used in the test-connection Pod. | `latest` |
| `extraDeploy` | Array of extra objects to deploy with the release | `[]` |
justusbunsi (Migrated from gitea.com) commented 2023-02-26 15:14:32 +00:00
Copy Link

Two occurrences.

- values.yml
+ values.yaml
Two occurrences. ```diff - values.yml + values.yaml ```
templates/_helpers.tpl
@ -129,4 +94,2 @@
{{- define "postgresql.dns" -}}
{{- if (index .Values "postgresql").enabled -}}
{{- printf "%s-postgresql.%s.svc.%s:%g" .Release.Name .Release.Namespace .Values.clusterDomain .Values.postgresql.global.postgresql.service.ports.postgresql -}}
justusbunsi (Migrated from gitea.com) commented 2023-02-26 15:17:34 +00:00
Copy Link

I know it's a PR for bumping the postgres version and nothing more. Do you fancy adding some simple unittests that ensure the mapping in our values.yaml works with the one the dependent chart expects? It should help with future updates, detecting changes.

I know it's a PR for bumping the postgres version and nothing more. Do you fancy adding some simple unittests that ensure the mapping in our `values.yaml` works with the one the dependent chart expects? It should help with future updates, detecting changes.
justusbunsi (Migrated from gitea.com) commented 2023-02-27 09:15:30 +00:00
Copy Link

We could/should but then probably for all chart deps? This might take a bit of work. Let's track it in #409. I think this should not block this PR for now.

We could/should but then probably for all chart deps? This might take a bit of work. Let's track it in #409. I think this should not block this PR for now.
values.yaml
@ -170,3 +140,2 @@
className:
annotations:
{}
annotations: {}
justusbunsi (Migrated from gitea.com) commented 2023-02-26 15:19:37 +00:00
Copy Link

We should keep the empty array/object consistent with the other definitions. It's easier to read. (related to several changes)

We should keep the empty array/object consistent with the other definitions. It's easier to read. (related to several changes)
justusbunsi (Migrated from gitea.com) commented 2023-02-27 09:11:29 +00:00
Copy Link

Yeah sorry for this, hitting the auto-format one time and then continueing brings trouble 😆️

Yeah sorry for this, hitting the auto-format one time and then continueing brings trouble 😆️
justusbunsi (Migrated from gitea.com) reviewed 2023-02-27 09:16:21 +00:00
justusbunsi commented 2023-03-07 16:11:50 +00:00 (Migrated from gitea.com)
Copy Link

Adding "breaking" label as it is a major bump for the database chart.

Adding "breaking" label as it is a major bump for the database chart.
justusbunsi (Migrated from gitea.com) reviewed 2023-03-22 16:08:18 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

Please see my comments below.

Please see my comments below.
Chart.lock
@ -13,3 +10,1 @@
version: 19.6.4
digest: sha256:462d513ac8ef7abfe26030fd2ea93eb79df167a861ebe09d6c58c7dcd5601e85
generated: "2024-11-30T00:41:29.178889496Z"
version: 12.2.6
justusbunsi (Migrated from gitea.com) commented 2023-03-22 17:24:02 +00:00
Copy Link

Apologies for not testing it earlier. In the meantime the latest version is 12.2.6. I think we can update to that version - I've already used that newer version during my tests. 😉

Apologies for not testing it earlier. In the meantime the latest version is 12.2.6. I think we can update to that version - I've already used that newer version during my tests. 😉
README.md
@ -814,4 +601,0 @@
request_headers = {
Accept = "application/json"
}
justusbunsi (Migrated from gitea.com) commented 2023-03-22 16:08:18 +00:00
Copy Link

I think this removal is a mistake. It must be kept, since Chart version 7.0.0 got released and had the breaking change of bumping Gitea to 1.18.1 and gpg key setup. See https://gitea.com/gitea/helm-chart/src/tag/v7.0.0#to-7-0-0

I think this removal is a mistake. It must be kept, since Chart version 7.0.0 got released and had the breaking change of bumping Gitea to 1.18.1 and gpg key setup. See https://gitea.com/gitea/helm-chart/src/tag/v7.0.0#to-7-0-0
README.md
@ -772,136 +598,29 @@ gitea:
podAnnotations: {}
justusbunsi (Migrated from gitea.com) commented 2023-03-22 17:21:50 +00:00
Copy Link

I just realized: Our description for postgresqlPassword is wrong. It is not the admin password. Based on the previously used Chart version 10.3.17 this would've been postgresqlPostgresPassword. What we configure instead is the actual additional user that is not the super-global admin of that instance. IMO, that's the desired state.

On the other hand: With Chart version 12.2.1 the key postgresql.global.postgresql.auth.postgresPassword is the password of that super-global admin and we have to use postgresql.global.postgresql.auth.password instead.

I just realized: Our description for `postgresqlPassword` is wrong. It is _not_ the admin password. Based on the previously used Chart version 10.3.17 this would've been `postgresqlPostgresPassword`. What we configure instead is the actual additional user that is not the super-global admin of that instance. IMO, that's the desired state. On the other hand: With Chart version 12.2.1 the key `postgresql.global.postgresql.auth.postgresPassword` is the password of that super-global admin and we have to use `postgresql.global.postgresql.auth.password` instead.
pat-s (Migrated from gitea.com) reviewed 2023-03-27 08:07:11 +00:00
README.md
@ -814,4 +601,0 @@
request_headers = {
Accept = "application/json"
}
pat-s (Migrated from gitea.com) commented 2023-03-27 08:12:22 +00:00
Copy Link

The confusion came up as someone assumed we need to always issue a major release for every gitea minor update, even if there are not breaking changes for the chart.

To clear the confusion, it might help to remove the statement of "this update bumps gitea to 1.18.1" as this somehow covers the underlying breaking change of the GPG key feature.

In other README changelog notes we never mentioned the Gitea minor release, hence I guess we should keep it at chart-related breaking changes to avoid confusion?

I've went ahead and did so, let me know what you thinK!

The confusion came up as someone assumed we need to always issue a major release for every gitea minor update, even if there are not breaking changes for the chart. To clear the confusion, it might help to remove the statement of "this update bumps gitea to 1.18.1" as this somehow covers the underlying breaking change of the GPG key feature. In other README changelog notes we never mentioned the Gitea minor release, hence I guess we should keep it at chart-related breaking changes to avoid confusion? I've went ahead and did so, let me know what you thinK!
README.md
@ -772,136 +598,29 @@ gitea:
podAnnotations: {}
pat-s (Migrated from gitea.com) commented 2023-03-27 08:07:11 +00:00
Copy Link

Good catch!

Good catch!
justusbunsi (Migrated from gitea.com) reviewed 2023-03-27 08:24:38 +00:00
justusbunsi (Migrated from gitea.com) approved these changes 2023-03-27 15:17:41 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

LGTM. I've fixed a similar port templating issue as I mentioned in https://gitea.com/gitea/helm-chart/pulls/412#issuecomment-733779 for MySQL.

LGTM. I've fixed a similar port templating issue as I mentioned in https://gitea.com/gitea/helm-chart/pulls/412#issuecomment-733779 for MySQL.
README.md
@ -814,4 +601,0 @@
request_headers = {
Accept = "application/json"
}
justusbunsi (Migrated from gitea.com) commented 2023-03-27 15:17:41 +00:00
Copy Link

Works for me how you proposed. 👍

Works for me how you proposed. 👍
justusbunsi (Migrated from gitea.com) reviewed 2023-03-27 15:44:18 +00:00
README.md
@ -814,4 +601,0 @@
request_headers = {
Accept = "application/json"
}
justusbunsi (Migrated from gitea.com) commented 2023-03-27 15:44:18 +00:00
Copy Link

This comment is related to https://gitea.com/gitea/helm-chart/pulls/391#issuecomment-734001.

This comment is related to https://gitea.com/gitea/helm-chart/pulls/391#issuecomment-734001.
pat-s commented 2023-03-27 17:12:06 +00:00 (Migrated from gitea.com)
Copy Link

Great, merging and moving onwards!

Great, merging and moving onwards!
👍 1
Sign in to join this conversation.
Reviewers
No Reviewers
techknowlogick
justusbunsi
Labels
Clear labels
has/backport
in progress
invalid
kind/breaking
kind/bug
kind/build
kind/dependency
kind/deployment
kind/docs
kind/enhancement
kind/feature
kind/lint
kind/proposal
kind/question
kind/refactor
kind/security
kind/testing
kind/translation
kind/ui
need/backport
priority/critical
priority/low
priority/maybe
priority/medium
reviewed/duplicate
reviewed/invalid
reviewed/wontfix
skip-changelog
status/blocked
status/needs-feedback
status/needs-reviews
status/wip
upstream/gitea
upstream/other
No Label
kind/breaking
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#391
No description provided.
Delete Branch "refs/pull/391/head"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?