-
Notifications
You must be signed in to change notification settings - Fork 401
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
App Submission: ONLYOFFICE Document Server for Nextcloud #1288
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really awesome work on this @a4004. Your onlyoffice-nextcloud-web
frontend is excellent, and instructions are super clear 👌
I've left some suggestions below, mostly around how we can simplify the environment variables/exports script.
onlyoffice-nextcloud/umbrel-app.yml
Outdated
manifestVersion: 1 | ||
id: onlyoffice-nextcloud | ||
category: files | ||
name: ONLYOFFICE Document Server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like they call themselves ONLYOFFICE Docs
: https://github.com/ONLYOFFICE/DocumentServer
I'm not familiar with ONLYOFFICE, so I'll defer to you here. But an added benefit to shortening the name is that it will show up without an ellipses on the homescreen:
vs
environment: | ||
- DOCS_ADDRESS=http://$DEFAULT_INTERFACE_IP:$DOCSERVER_PORT | ||
- DOCS_INTERNAL_ADDRESS=http://$DEFAULT_INTERFACE_IP:$DOCSERVER_PORT | ||
- NEXTCLOUD_INTERNAL_ADDRESS=http://$DOCKER_INTERFACE_IP:$APP_NEXTCLOUD_PORT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify this a bit and avoid needing to run ip-related commands in the exports.sh
. I'm not very familiar with the Nextcloud/ONLYOFFICE ecosystem, so please let me know if anything I suggest here doesn't make sense or has unintended consequences:
DOCS_ADDRESS=http://$DEFAULT_INTERFACE_IP:$DOCSERVER_PORT
I'm thinking that most users will be accessing Nextcloud viaumbrel.local
not their IP address, so if they are using their local IP address in the nextcloud/onlyoffice config they will get hit with cors errors when clicking on a document in Nextcloud fromhttp://umbrel.local:8081
and it tries to make a request to$DEFAULT_INTERFACE_IP:5672
.
Shall we change this to DOCS_ADDRESS=http://$DEVICE_DOMAIN_NAME:$DOCSERVER_PORT
(which will be http://umbrel.local:5672) and then include instructions saying that if a user is accessing Nextcloud from a different hostname/ip, they should change make the appropriate change to ONLYOFFICE Docs address
in their Nextcloud settings?
**Edit: sorry @a4004 I sent you down the wrong path by accidently including :$DOCSERVER_PORT
after the container name below **
-
DOCS_INTERNAL_ADDRESS=http://$DEFAULT_INTERFACE_IP:$DOCSERVER_PORT
This is for internal requests to thedocumentserver
container, so we can just use the container name here and Docker's internal DNS will resolve it to the correct IP address. So I think we can change this toDOCS_INTERNAL_ADDRESS=http://onlyoffice-nextcloud_documentserver_1
.
The user will then copy-pastehttp://onlyoffice-nextcloud_documentserver_1
into the ONLYOFFICE settings in Nextcloud. -
NEXTCLOUD_INTERNAL_ADDRESS=http://$DOCKER_INTERFACE_IP:$APP_NEXTCLOUD_PORT
I think we can get away without the export.sh ip command here as well. We actually have access to the gateway IP for the umbrel docker network as an env var. Check it out: https://github.com/getumbrel/umbrel/blob/570acdabcab13ac237fba992f491d126fff30810/packages/umbreld/source/modules/apps/legacy-compat/app-script#L10
So we should be able to useNEXTCLOUD_INTERNAL_ADDRESS=http://$GATEWAY_IP:$APP_NEXTCLOUD_PORT
which will yieldhttp://10.21.0.1:8081/
for the user to copy-paste into the ONLYOFFICE settings in Nextcloud.
So a user would end up with this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nmfretz Is it not better to use something like http://nextcloud_web_1:8081
? We are using container hostname reference on torrenting apps like Prowlarr, Radarr and Sonarr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually a good point.
I have tested both http://nextcloud_web_1/
and http://nextcloud_web_1:8081/
but it doesn't appear to work for some reason. Nextcloud is throwing an error Error when trying to connect (Error occurred in the document service: Error while downloading the document file to be converted.) (version 8.1.1.26)
.
Perhaps I'm doing something wrong here or it's not set up for this type of communication, not too sure but I agree with the idea of using container hostnames over IPs for sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joaovictor-local @a4004, ya the issue here is that the Nextcloud app doesn't have the web
container name as a trusted proxy in its config.php
, so instead we are using an IP that it does trust (e.g., 10.21.0.1
or 172.17.0.1
).
umbrel-apps/nextcloud/docker-compose.yml
Line 47 in 49580db
- NEXTCLOUD_TRUSTED_DOMAINS=${APP_DOMAIN}:${APP_NEXTCLOUD_PORT} ${APP_HIDDEN_SERVICE} ${DEVICE_HOSTNAME}:${APP_NEXTCLOUD_PORT} ${APP_NEXTCLOUD_LOCAL_IPS} |
umbrel-apps/nextcloud/exports.sh
Lines 3 to 4 in 49580db
local_ips=$(hostname --all-ip-addresses 2> /dev/null) || local_ips="" | |
export APP_NEXTCLOUD_LOCAL_IPS="${local_ips}" |
For this to work, the Nextcloud config.php would have to include nextcloud_web_1
under trusted_domains. Then in the field for Server address for internal requests from ONLYOFFICE Docs
a user would use http://nextcloud_web_1
, without a port specified since we want the default port 80 inside the web container (not 8081).
I do really like the simplicity of using container names @joaovictor-local, so we could consider the following:
- Update the Nextcloud app to include
nextcloud_web_1
in the config.php - Change the env var in ONLYOFFICE Docs to
NEXTCLOUD_INTERNAL_ADDRESS=http://nextcloud_web_1
- Include a note in the
a4004/onlyoffice-nextcloud-web
UI (or maybe just the app description) telling users to update Nextcloud if they haven't.
What do you guys think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I have updated Nextcloud to allow connection via nextcloud_web_1 #1297
restart: on-failure | ||
stop_grace_period: 1m | ||
ports: | ||
- 5672:80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might as well use - "${DOCSERVER_PORT}:80"
from exports.sh
onlyoffice-nextcloud/exports.sh
Outdated
#!/usr/bin/env bash | ||
|
||
export DOCSERVER_PORT=5672 | ||
export APP_DOCSERVER_PORT=3014 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is export APP_DOCSERVER_PORT=3014
still needed, or can it be removed?
onlyoffice-nextcloud/exports.sh
Outdated
export DEFAULT_INTERFACE_IP=$(ip addr show $(ip route | grep default | awk '{print $5}') | grep 'inet ' | awk '{print $2}' | cut -d/ -f1) | ||
export DOCKER_INTERFACE_IP=$(ip addr show docker0 | grep 'inet ' | awk '{print $2}' | cut -d/ -f1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we end up going with the simplifications here #1288 (comment), then we can remove these exports.
Thanks for the suggestions @nmfretz! Happy with all of these:
I did some testing with the new configuration and there does seem to be an issue with using the container name for the ONLYOFFICE Docs address for internal requests from the server property.
Hitting the So as a quick solution, we might be able to just use the |
To elaborate on the name change, it is a good idea and will improve the appearance of the app, but I also think some improvements can be made to the whole app in light of that. I'm also consdering adding a startup screen, similar to how the Bitcoin Node and Lightning Node apps have it with a progress bar, so if a user clicks on the ONLYOFFICE Docs app, they will be redirected to the actual app within Nextcloud in a few seconds with the opportunity to cancel and view the setup instructions instead. Ideally, there would be some configuration detection mechanism so it doesn't redirect upon the very first launch and only when it knows Nextcloud has been configured, but I think that's for a future update for now. |
Tested it on my instance and all looks good now. Should be ready to go. |
Thanks @a4004!
Looks like you may have accidentally pushed the changes to a different branch https://github.com/a4004/umbrel-apps/commit/3daa6b91fcbb5b2c147f159e3b5ce2b7c40fd03b
Just noting for our records that
This is a cool idea, but honestly I think there's a lot of benefit to having this app just stay on the instructions page because:
That being said, this is your app, so it's totally up to you! Also, @joaovictor-local @a4004 I've responded here with a suggestion on how we could use the nextcloud web container name: #1288 (comment) |
🎉 Linting finished with no errors or warnings 🎉Thank you for your submission! This is an automated linter that checks for common issues in pull requests to the Umbrel App Store. Please review the linting results below and make any necessary changes to your submission. Linting Results
Legend
|
Thanks for pushing the changes @a4004. Absolutely fantastic work on this app! Going live to the app store 🎉. FYI, I have done the following to finalize:
|
Thank you very much for making ONLYOFFICE available to Umbrel and, furthermore, associated to NextCloud ❤️ 🎉 This PR closes #1086 |
App Submission
ONLYOFFICE Document Server
256x256 SVG icon
Gallery images
I have tested my app on: