Latest template updates break DB Operator

dannysauer commented 2 months ago

The last commit (https://github.com/diggerhq/helm-charts/commit/2cf4b153c66b40dcb1f556a8756025fdb8950b9b#diff-204d7782307a1567576db307ef282e33ef9a56ab6024b064960f777651b15acaL35-R49) / #10 removes the ability to use an existing secret to set multiple environment variables and has broken conditional logic.

The code which was replaced worked with a secret like this, generated by db-operator:

apiVersion: v1
data:
  DATABASE_URL: redacted
  POSTGRES_DB: redacted
  POSTGRES_PASSWORD: redacted
  POSTGRES_USER: redacted
kind: Secret
metadata:
  creationTimestamp: "2024-09-07T03:27:07Z"
  labels:
    app.kubernetes.io/managed-by: db-operator
    kinda.rocks/used-by-kind: Database
    kinda.rocks/used-by-name: digger-db
  name: digger-db-credentials
  namespace: infra-digger
  ownerReferences:
  - apiVersion: kinda.rocks/v1beta1
    kind: Database
    name: digger-db
    uid: 17c1d193-77a2-4739-8324-9894d3c1fca8
  resourceVersion: "345098783"
  uid: 07b60dc5-03e3-447a-ab12-93a0107f14e1
type: Opaque

The new code only supports POSTGRES_PASSWORD, breaking anything using the previous existing secret logic. It also has an initial conditional followed by an else and then another conditional; the last two segments are swapped. So if someone were to specify an existing secret using Values.digger.postgres.existingSecretName and also set postgres_enabled to false (it now weirdly defaults to true, which probably should not be the default and should definitely be more loudly communicated in the changelog), the chart would render two valueFrom lines, resulting in invalid yaml.

dannysauer commented 2 months ago

Without adding too much commentary because I know I'm in a mood from tracking this all down... Seriously. The changes made there are not patch level. "Major version = 0" doesn't mean it's cool to completely break backwards compatibility in multiple places without adding any release notes at all. That PR broke my setup in at least two completely avoidable places, adding only functionality which was already achievable with the pre-PR chart. And, again, zero documentation of all the new values. :/

motatoes commented 2 months ago

Hey thanks for reporting these (#11 and #13)!

Not gonna lie our exposure to helmcharts has been limited before digger so we clearly messed up.

We wanted to improve a few of the defaults but we did a bad job and testing for backwards compatability, I will prioritise a fix for these items to make it work out from this point on and document that the latest release has broke some older defaults

motatoes commented 3 weeks ago

resolved in #14

diggerhq / helm-charts

Latest template updates break DB Operator #12