jitsi-contrib / jitsi-helm

A helm chart to deploy Jitsi to Kubernetes
MIT License
120 stars 65 forks source link

Introduce new customization options and restricted config filesystems #92

Closed at-platform24 closed 5 months ago

at-platform24 commented 8 months ago

with some small and some large changes.

A summary is:

at-platform24 commented 8 months ago

@spijet May you have a review please.

spijet commented 7 months ago

Hello @at-platform24!

Sorry for the huge delay. I've added some comments to the PR, please reply when you have time.

Do I understand it right that if the custom defaults file value is empty or unset, the ConfigMap field gets populated with a stub comment, and there will be no record for that file in volumeMounts?

Also, can you please elaborate on the replicas field for Prosody? Do you want to scale it up or to replace it with an external Prosody instance(s)?

at-platform24 commented 7 months ago

Do I understand it right that if the custom defaults file value is empty or unset, the ConfigMap field gets populated with a stub comment, and there will be no record for that file in volumeMounts?

That is correct. make template or make templateWithCustomDefaults can be tried to verify the volumenMounts.

 make template > /tmp/jitsi-default.yaml
 make templateWithCustomDefaults > /tmp/jitsi-custom.yaml
 diff /tmp/jitsi-* | tail -30

Do you want to scale it up or to replace it with an external Prosody instance(s)?

Our use case is where we can use the external prosody instance, however, having the full config available from the helm release make external prosody configuration teaks and validation mush simpler, as well as being able to quickly stop/ start different versions during upgrades, having a safe and fast rollback path where run in-cluster prosody.

Thank you for the thoughtful review, and excellent work in maintaining this chart.

spijet commented 7 months ago

Our use case is where we can use the external prosody instance, however, having the full config available from the helm release make external prosody configuration teaks and validation mush simpler, as well as being able to quickly stop/ start different versions during upgrades, having a safe and fast rollback path where run in-cluster prosody.

Hmm, maybe we could add a new option that disables the provision of Prosody (the StatefulSet, Service and whatnot) while keeping all the ConfigMaps and Secrets intact? I think it might be easier than explaining that the replicas field is not supposed to be used for actually scaling Prosody. :)

at-platform24 commented 7 months ago

Makes sense.

fixed this, along with adding a comment. However, have not removed full manifest creation since it is useful to know what all is needed by prosody. and resolved merge conflicts.

PR title is no longer relevant, but I guess that should not be a problem :smile:

spijet commented 7 months ago

Oh, I just realized that I forgot to actually submit the review. 🤦‍♂️

Sorry, I'll update the comments and submit the review in a moment.

at-platform24 commented 7 months ago

One question about version. This changeset does not add any breaking changes from previous one, should this be a patch version bump or minor version bump?

spijet commented 7 months ago

should this be a patch version bump or minor version bump?

Let's do a minor version bump (v1.3.8 -> v1.4.0). This PR adds new functionality to the chart, so it makes sense to bump the minor version, even though there are no breaking changes.

spijet commented 7 months ago

Also, since the Makefile and the custom configuration examples are not going to be rendered by Helm and applied to the k8s cluster during chart install, it makes sense to add the Makefile and example-configurations directory to the .helmignore file. Can you do that inside the PR, please?

at-platform24 commented 7 months ago

All changes applied. Feel free to make any changes needed that you find useful for the broader user community.

spijet commented 6 months ago

Sorry for the delay. :( I'll review the revised changes in the coming days and will let you know if everything is OK.

spijet commented 5 months ago

Once again, sorry for the delay. The Christmas / New Year holidays are a busy period here. 😅 I'll review it now and will let you know if there's anything else we need to edit before merging.

spijet commented 5 months ago

I just merged #99, so keep an eye out for any possible conflicts and be ready to rebase if needed. 👀

at-platform24 commented 5 months ago

I just merged #99, so keep an eye out for any possible conflicts and be ready to rebase if needed. 👀

luckily for me no conflicts occurred on merge. I hope we are ready for finally merging the MR.

spijet commented 5 months ago

Let's do one final push with the "bundled vs external" Prosody knob and the ConfigMap permissions and I'll merge it right away. :)

spijet commented 5 months ago

Yep, looking good to me! Merging.

Thank you for your time and once again sorry for the delays!