[installer]: add annotation to make DB resources restart if changes #8547
Conversation
mrsimonemms
force-pushed
the
sje/db-annotation
branch
from
a5c63c5 to
bfe8dc0
Compare
Codecov Report
@@ Coverage Diff @@
## main #8547 +/- ##
=========================================
- Coverage 12.31% 7.55% -4.77%
=========================================
Files 20 31 +11
Lines 1161 2171 +1010
=========================================
+ Hits 143 164 +21
- Misses 1014 2004 +990
+ Partials 4 3 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@mrsimonemms makes absolute sense to add this! Should we extend this to all DB connection information? Like user and password as well? |
| { | ||
| Name: "DATABASE_TYPE", | ||
| Value: func() string { | ||
| if pointer.BoolDeref(ctx.Config.Database.InCluster, false) { |
There was a problem hiding this comment.
I understand that it's not required, but: would it make sense to share code between all DB-dependant deployments? 🤔
There was a problem hiding this comment.
As it's just this one part, I took the view that a little bit of duplication was ok (@csweichel beat that into me 😄).
As you're now the owners of this code, I'm happy to write a function inside common to generate the annotations if you'd prefer
There was a problem hiding this comment.
wrt duplication: Yeah, in this case it might actually be counter-productive, as the only dependency is within the deployment itself. If we changed a single function, we might inadvertently change other as well.
I think what I'm missing most is a method name/comment that conveys the intention: what we do, and why these values are the important ones to establish identity.
There was a problem hiding this comment.
Yeah, I'll happily add a comment
There was a problem hiding this comment.
Comment added to both instances
That would be nice. The only problem is, except for the in-cluster DB (who's details never change), we don't actually have access to the data. The external/cloudsql creds are stored in a secret which we just refer to |
Can we hash the secret then? 🤔 |
I don't think so. Because the Installer doesn't have access to the cluster at render time, we don't know what the contents of the secrets are to create a hash from |
mrsimonemms
force-pushed
the
sje/db-annotation
branch
from
bfe8dc0 to
128bead
Compare
|
@geropl what are your thoughts on this? At this stage, it's practical to get the credentials to reload when change, just the DB type. As I see it, we have two options:
|
Thx for the 💡 , makes sense.
That 💯 won't work for for SaaS. Let's merge this as-is. |
Description
Add the database type (in-cluster, external, cloudsql) as an annotation on the long-running resources that connect to the database. This is to force a restart of these resources if the database type is changed.
This has become important to support with KOTS. This can cause a bug where the "login with GitHub" button never shows.
How to test
Release Notes
Documentation