nextcloud / helm

A community maintained helm chart for deploying Nextcloud on Kubernetes.

GNU Affero General Public License v3.0

295 stars 258 forks source link

Feature/client push #551

Open AndreKoepke opened 2 months ago

AndreKoepke commented 2 months ago

Pull Request

Description of the change

Add a container for client_push app. It allows websocket-connections for updates instead of polling. See notify_push

Benefits

notify_push is usable

Possible drawbacks

Nextcloud-Chart must be started without notify_push, so it can be installed.
Then Next-Chart can be started with notify_push
After that, notify_push must be manually activated by running sudo -u www-data ./occ notify_push:setup https://NEXTCLOUD_HOST/push

Applicable issues

fixes #109

Additional information

Checklist

[ ] DCO has been signed off on the commit.
[ ] Chart version bumped in Chart.yaml according to semver.
[ ] (optional) Variables are documented in the README.md

AndreKoepke commented 2 months ago

Can we install apps with commands? If so, it can be added as default.

Leptopoda commented 1 month ago

Can we install apps with commands? If so, it can be added as default.

there is occ app:install notify_push https://docs.nextcloud.com/server/stable/admin_manual/configuration_server/occ_command.html#apps-commands-label

wrenix commented 2 weeks ago

I really like the idea to run notify_push.

But please in a different way:

extra deployment (like for metrics)
extra service and ingress with Additional path
improve service monitor to track that metrics also

The reason:

On this way, we could scale nextcloud indepenten fron push client
And do not maintain nginx or Apache code, that could handle ingress-controller.

Another reason (i need to verify / hope that it exists), we could use a other image for the client push (without any distro inside thanks Rust) and track a more beautiful/helm-like way the version. (That image exists but is not official yet: https://github.com/nextcloud/notify_push/issues/106#issue-929457837) PS: it will also solve the CPU architecture problem (the notify_push image should just build in all targets under the same container label).

i start to write, what i mean under: #581 if you like to test it or use that as first state to improve your PR take it

AndreKoepke commented 2 weeks ago

i start to write, what i mean under: #581 if you like to test it or use that as first state to improve your PR take it

I like your changes. I'm running my variant since I opened the PR, but I will switch to your variant in the next days.
Should we (or I) close my PR?

wrenix commented 2 weeks ago

Till there is a solution merged i would keep it open in your place.