Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: client push #551

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

AndreKoepke
Copy link

@AndreKoepke AndreKoepke commented Apr 2, 2024

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

Additional information

Checklist

@AndreKoepke
Copy link
Author

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

image: {{ include "nextcloud.image" . }}
imagePullPolicy: {{ .Values.image.pullPolicy }}
command:
- custom_apps/notify_push/bin/x86_64/notify_push
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard-coded x86_64

Signed-off-by: Andre Köpke <github@akop.online>
@Leptopoda
Copy link
Member

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
Copy link
Collaborator

wrenix commented Jun 9, 2024

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: nextcloud/notify_push#106 (comment)) 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
Copy link
Author

AndreKoepke commented Jun 12, 2024

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
Copy link
Collaborator

wrenix commented Jun 14, 2024

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

@jessebot jessebot changed the title Feature/client push Feature: client push Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] High Performance File Backend
4 participants