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

Latest template updates break DB Operator #12

Closed
dannysauer opened this issue Sep 7, 2024 · 3 comments
Closed

Latest template updates break DB Operator #12

dannysauer opened this issue Sep 7, 2024 · 3 comments

Comments

@dannysauer
Copy link
Contributor

The last commit (2cf4b15#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
Copy link
Contributor Author

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
Copy link
Contributor

motatoes commented Sep 9, 2024

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
Copy link
Contributor

resolved in #14

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

No branches or pull requests

2 participants