Configure external redis #30
Conversation
|
Not an expert, so I'll defer to @tamcore to review this. |
|
@tamcore is there any news? |
| @@ -115,18 +120,14 @@ mysql: | |||
| persistence: | |||
| enabled: false | |||
|
|
|||
| # NOTE: When using multiple replicas, you must configure broadcast.type and broadcast.address | |||
| # NOTE: When using multiple replicas, you must configure broadcast.enabled | |||
There was a problem hiding this comment.
I'd reword the entire block here. Because now there are essentially 3 options
- just set
redis.enabled=true(as seen in line 125) - manually set
traccar.broadcast.enabledandtraccar.broadcast.address - when using
configOverride, by configuringbroadcast.typeandbroadcast.address
There was a problem hiding this comment.
In my opinion you should design the whole chart differently and use only the environment variables and not the conf generation in the configmap, this would simplify everything.
Unfortunately, I don't have the time to review the whole chart and try to eliminate the whole thing.
There was a problem hiding this comment.
Using a ConfigMap is perfectly fine and cleaner than having everything defined as individual env variables. (And envFrom would reference a CM / Secret again anyways.) But if you want to have a look at #28, I've started a bit of a different approach there, which would allow everyone to just pass every desired configuration parameter as part of the yaml dict. I just don't have the time to finish it.
Anyhow, my comment here regarding
traccar-helm/charts/traccar/values.yaml
Lines 123 to 125 in e3d6cf7
| {{- end }} | ||
| {{- if and (.Values.traccar.broadcast.enabled) (not (index .Values "redis-ha").enabled) }} | ||
| <entry key='broadcast.type'>redis</entry> | ||
| <entry key='broadcast.address'>{{ .Values.traccar.broadcast.address }}</entry> |
There was a problem hiding this comment.
Might want to change this to {{ required "A Redis address must be configured!" .Values.traccar.broadcast.address }} or something similar, to avoid issues where . Values.traccar.broadcast.enabled could be enabled without an address configured.
There was a problem hiding this comment.
I wouldn't change it, I don't think it's a good idea to add exceptions, plus to date it hasn't been configured/thought out to make it as you suggested.
No description provided.