helm icon indicating copy to clipboard operation
helm copied to clipboard

Feature: client push

Open AndreKoepke opened this issue 1 year ago • 5 comments

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

  • [x] 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 avatar Apr 02 '24 09:04 AndreKoepke

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

AndreKoepke avatar Apr 02 '24 09:04 AndreKoepke

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

Leptopoda avatar May 02 '24 07:05 Leptopoda

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

wrenix avatar Jun 09 '24 21:06 wrenix

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?

AndreKoepke avatar Jun 12 '24 06:06 AndreKoepke

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

wrenix avatar Jun 14 '24 21:06 wrenix