fix the default values that broke 1.5 #15
Conversation
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.digger.postgres.existingSecretName }} | ||
| key: {{ .Values.digger.postgres.existingSecretKey }} | ||
| {{- else }} | ||
| valueFrom: | ||
| secretKeyRef: |
There was a problem hiding this comment.
I can't leave a comment outside of the diff, but the succeeding if block (lines 49-54) needs to also be removed. It's what breaks the template.
There was a problem hiding this comment.
rewording for maybe clarity?
The "else" and second if condition with negation of the preceding if conditions result in two valuesFrom blocks when and .Values.digger.postgres.existingSecretName (not .Values.postgres.enabled)
There was a problem hiding this comment.
Yeah, sorry. I had already explained on Slack (and showed the diff on my fork that this PR is based off of) so I figured I didn't need to reiterate it.
There was a problem hiding this comment.
Making sense, removed!
charts/digger-backend/values.yaml
Outdated
| # DB connection string should be in a key names DATABASE_URL | ||
| # compatible with https://github.com/kloeckner-i/db-operator, for example | ||
| existingSecretName: "new-pg-creds" | ||
| existingSecretName: "" | ||
| existingSecretKey: "postgres-password" |
There was a problem hiding this comment.
I think the existingSecretKey and the example in the comment should probably match. Obviously I'm partial to DATABASE_URL since that was my example, but consistency is more important. :) Also, I guess I misspelled "named" in that comment. :/
There was a problem hiding this comment.
Actually, this implementation is still somewhat incompatible with the DB Operator mechanism.
When using db-operator to create the database, a secret is created which contains the entire connection URL. One can set up a Secret Template with multiple values, as in the docs, but typically it's preferable to just let the operator or other secret manager (I've used AWS systems manager secret driver and RDS for this before) build the whole connection string and inject that into the namespace.
So the outcome of the prior change is that I'd have to use something like kustomize to patch a secret with some other environment variables post-render. The change introduced in #7 and #8 provides a way for DATABASE_URL to be brought in directly from an external secret. Basically the postgres.existingSecretName was hijacked in #10 to change from actually containing the full URL secret to setting an entirely different environment variable used later to build the connection string. What ideally should've happened was to create two separate configuration mechanisms: one for specifying the connection string components, and a separate one for specifying the complete string.
There was a problem hiding this comment.
So I improved the comment, regarding the DB operator, my basic understanding is that we currently accept individual DB URL string components and form it ourselves. Perhaps to support accepting entire string as a secret we can try to override the value here and accept it as a secret rather than an env variable
But I think it i something for another PR!
There was a problem hiding this comment.
Accepting the whole URL worked before #10. ;)
I'll submit support again in a new PR since I strongly suspect I'm the only one who currently uses it.
| valueFrom: | ||
| secretKeyRef: | ||
| name: {{ .Values.digger.postgres.existingSecretName }} | ||
| key: {{ .Values.digger.postgres.existingSecretKey }} | ||
| {{- else }} |
There was a problem hiding this comment.
This conditional applies if postgres is not enabled, which is a bit weird.
There was a problem hiding this comment.
I think if the "test purposes" postgres is not enabled (postgres.enabled) then we default to the values of some managed postgres so connection strings are fetched from (digger.postgres.xxxx). I dunno if we should add extra "enabled" flag under digger.postgres since it is already the fallback I guess
There was a problem hiding this comment.
Oh, sure. That's "built-in postgres enabled". I'm gonna blame the variable name for not being clear, but really it's just that I forgot postgres is always used. 😂
This is fine then. My bad.
No description provided.