-
Notifications
You must be signed in to change notification settings - Fork 3
fix: external PostgreSQL & Redis configurations #32
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
base: main
Are you sure you want to change the base?
Conversation
…rovide usage examples for database and Redis configurations
…on external database settings
| # before rolling out the rest of the stack. | ||
| # - If `database.external = false`, this chart creates the Postgres Deployment/PVC and | ||
| # must wait for them to be ready, so we run migrations as a *post-install* hook. | ||
| {{- if .Values.database.external }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Migration Hook Timing Needs Redis Awareness
The migrations job runs as a pre-install hook when database.external is true, but it has an init container that waits for Redis. When Redis is internal (redis.external = false) and the database is external (database.external = true), the migrations job runs before the Redis deployment is created, causing the wait-for-redis init container to fail. The hook timing should depend on both database.external and redis.external being true.
| # before rolling out the rest of the stack. | ||
| # - If `database.external = false`, this chart creates the Postgres Deployment/PVC and | ||
| # must wait for them to be ready, so we run migrations as a *post-install* hook. | ||
| {{- if .Values.database.external }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the job migration depend on both the database and Redis being available ?
If we go with this logic we are checking only the status of database isn't it? Like lets say a user is using RDS but want to run redis inside kubernetes cluster - in that case, this condition will pass but migration job since it's pre-install probably will cause issues due to how helm works.
I suggest we rather do :
{{- if and .Values.database.external .Values.redis.external }}
"helm.sh/hook": "pre-install,pre-upgrade"
{{- else }}
"helm.sh/hook": "post-install,post-upgrade"
{{- end }}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additional suggestion from simulation.
-
The migrations Job uses a pre-install hook when
database.external = true -
Pre-install hooks execute before any regular resources are created
-
The Job has a
wait-for-redisinit container that requires Redis to be available -
Internal Redis Deployment is a regular resource, not created until after hooks complete
-
Init container waits indefinitely for Redis that doesn't exist yet
-
Migration Job fails, blocking entire Helm installation
-
Root cause: Hook timing logic only checks
database.external, ignoringredis.external
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually do not understand why do we have this as a pre-install hook?
was there any reason for this design?
connorv001
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only remaining edge case I’m worried about is when:
database.external = true(e.g. RDS)redis.external = false(Redis inside this chart)
With the current logic, the migrations Job runs as a pre-install hook based solely on database.external, but the Job still has the wait-for-redis init container.
Changes made:
Values.global.external.enabledin favor of.Values.database.externaland or.Values.redis.externalglobal.external.enabledno longer affects whether these Deployments/PVCs are created.Addressed #31 issue.
Note
Cursor Bugbot is generating a summary for commit 1ce8fa4. Configure here.