jitsi-contrib/jitsi-helm

Prosody is always restart

wrenix opened this issue · 13 comments

wrenix commented

Prosody is always restart on helm-chart run, even no values or chart-version changes.

the anti-pattern is here:
https://github.com/jitsi-contrib/jitsi-helm/blob/3744d68b90c0786d2b458d5af7357fc85ed562c3/charts/prosody/templates/statefulset.yaml#L24C73-L25C5


Therefore the helm-chart should use an hash from the values (instatt of an timestamp):

https://helm.sh/docs/chart_template_guide/function_list/#cryptographic-and-security-functions

e.g. (or an subpart of Values and multiple hash):

labels:
hash: {{ toYaml .Values | sha256sum }}

spijet commented

Hello @wrenix!

Sorry for not answering earlier. Thank you for your PR! I'll review it tomorrow and will let you know if it needs any additional edits before merging.

wrenix commented

thanks, i split it up for an easier review (there are some strange and not well readable expression - so the cleanup is here #86 )

wrenix commented

any update?

wrenix commented

is everything fine with your @spijet ? Or just no time left to speed?

spijet commented

@wrenix, I'm terribly sorry for the delay — had a lot going on IRL.

I have two minor comments about the #84:

  1. Can you confirm that it plays nice with setups where people don't explicitly set any passwords or secrets, and so they get re-generated on every deploy? (I assume it does, since the secret hash would be different, but some caution won't hurt)
  2. Can you please add the si.jit.meet/ prefix to both annotations to emphasize that it's set by us (Chart developers) and required for smooth operation of the project?

As for #86, I'll have to study it a bit more. I see that you've removed a couple of tpl calls here and there, and some of them may have been used to create semi-dynamic stuff similar to what you can see in Bitnami charts, where you can use templating even inside values. I wanted to implement full-blown templating support for Jitsi chart, but got reluctant and lazy at the same time (reluctant to pull Bitnami's common library chart as a dependency, lazy to implement it myself 😅).

spijet commented

I had some time to look at #86. Thank you very much! It really makes the chart much more readable and consistent than it was before. I added some comments to it and hopefully will add more tomorrow, after I look at it again with a fresh head. :)

wrenix commented

lets only talk here about #84 (for #86 in there PR).

  1. it is restarted on every change in env, secretEnvs and extraEnvFrom (like any other value inside of the template section). Only on change of values which are references inside of extraEnvFrom, that should not be an job of the helmchart, therefore exists: .Values.podAnnotations
  2. done
spijet commented

Thank you very much! Can I merge #84 now or should I wait until we finalize #86?

wrenix commented

lets merge it now

wrenix commented

do not forget to increase helm-chart version

spijet commented

Done! Will file a Release now.