chart: allow to use external redis instance #20322
Conversation
chart/templates/configmap-env.yaml
Outdated
| @@ -12,7 +12,7 @@ data: | |||
| {{- end }} | |||
| DB_NAME: {{ .Values.postgresql.auth.database }} | |||
| DB_POOL: {{ .Values.mastodon.sidekiq.concurrency | quote }} | |||
| DB_PORT: "5432" | |||
| DB_PORT: {{ .Values.postgresql.auth.port | default "5432" | quote }} | |||
There was a problem hiding this comment.
There's a separate PR tackling this specifically so it might be a good idea to drop it from here
(and I guess technically the port shouldn't be in auth. database is in there because it's sort of "related" to the authentication being used)
I Think port should probably be on the same level as the external hostname which is slightly confusing, but the place bitnami has it is bizarre and you'll probably only change this if you're also specifying the hostname
There was a problem hiding this comment.
Do you have a link to the other PR @deepy ? I was going to raise a PR to set a non-standard Postgres port too, would be great to track it.
| REDIS_HOST: {{ template "mastodon.redis.fullname" . }}-master | ||
| REDIS_PORT: "6379" | ||
| {{- else }} | ||
| REDIS_HOST: {{ .Values.redis.hostname }} |
There was a problem hiding this comment.
Since this is required if redis is disabled it'd be nice to add validation for it, perhaps something like: required "When the redis chart is disabled .Values.redis.hostname is required" .Values.redis.hostname?
|
This has merge conflicts now. |
ineffyble
added
the
deployment
|
Do we only need to remove the (now redundant through #20370) PostgreSQL port portions of this PR in order to resolve the merge conflicts? Extremely keen to see this PR launch! |
|
Probably yeah, it's pretty clean so should be easy to fix |
This reverts commit cedf3f4.
|
I reverted the commit for the postgres port. sorry for the late response, was on holiday |
|
At the risk of being slightly off-topic, absolutely no need to apologise @norman-zon! 2022 has been a rough year, we've all needed a break! Mental health should absolutely come first <3 |
chart/templates/configmap-env.yaml
Outdated
| @@ -31,8 +31,13 @@ data: | |||
| MALLOC_ARENA_MAX: "2" | |||
| NODE_ENV: "production" | |||
| RAILS_ENV: "production" | |||
| {{- if .Values.redis.enabled }} | |||
| REDIS_HOST: {{ template "mastodon.redis.fullname" . }}-master | |||
| REDIS_PORT: "6379" | |||
There was a problem hiding this comment.
isn't this already covered by both {{ .Values.redis.port | default "6379" | quote }} and also the new redis.port defaulting to 6379?
There was a problem hiding this comment.
It is, and I have to apologize 😣 I made a comment yesterday based on the current changes on main but after taking a closer look at @norman-zon's code I realized that his implementation was better. I deleted the message soon afterwards hoping no one had seen it, but I guess it made it to some mailboxes.
This reverts commit 66e50cb.
|
Moving the Helm chart out to mastodon/chart |
|
Replaced by mastodon/chart#6 |
This PR adds the capability to use an external redis instance, instead of the bundled one, to the helm chart.
I tried to be consistent with syntax used for the database.