-
Notifications
You must be signed in to change notification settings - Fork 93
chore: simplify dsp setup #386
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Julius Knorr <[email protected]>
|
|
||
| appapi-dsp: | ||
| image: ghcr.io/nextcloud/nextcloud-appapi-dsp:release | ||
| container_name: nextcloud-appapi-dsp-http |
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.
The container name can probably also be dropped
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.
can be useful for easy docker inspect sometimes but yeah not really required
|
|
||
| ```bash | ||
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP HTTP" docker-install http "nextcloud-appapi-dsp-http" "http://nextcloud.local" --net=master_default --set-default | ||
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP" docker-install http "appapi-dsp:2375" "https://nextcloud.local" --set-default --haproxy_password some_secure_password --net nextcloud_default |
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.
The docs are not fully up to date yet, juts pasted my command here to keep track of it.
Also this should be this one as there is no compose project name set by default:
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP" docker-install http "appapi-dsp:2375" "https://nextcloud.local" --set-default --haproxy_password some_secure_password --net nextcloud_default | |
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP" docker-install http "appapi-dsp:2375" "https://nextcloud.local" --set-default --haproxy_password some_secure_password --net master_default |
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.
Pull Request Overview
This PR simplifies the DSP setup for development by using a single HTTP container instead of separate HTTP and HTTPS containers.
- Updates documentation to refer to a new container host and adjusts the OCC daemon registration command.
- Removes the HTTPS container configuration from the docker-compose file.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| docs/services/app_api.md | Updates the host parameter and OCC registration command for DSP. |
| docker-compose.yml | Eliminates the HTTPS container configuration for the DSP. |
Comments suppressed due to low confidence (1)
docker-compose.yml:1089
- [nitpick] The container name 'nextcloud-appapi-dsp-http' may be confusing since the documentation now refers to the host as 'appapi-dsp'. Consider renaming it to match the updated naming convention if that is the intended behavior.
container_name: nextcloud-appapi-dsp-http
|
|
||
| ```bash | ||
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP HTTP" docker-install http "nextcloud-appapi-dsp-http" "http://nextcloud.local" --net=master_default --set-default | ||
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP" docker-install http "appapi-dsp:2375" "https://nextcloud.local" --set-default --haproxy_password some_secure_password --net nextcloud_default |
Copilot
AI
Apr 1, 2025
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.
The OCC registration command now uses 'https://nextcloud.local' which contradicts the PR description stating that internal communication should use plain HTTP. Consider updating the URL to 'http://nextcloud.local' to ensure consistency.
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP" docker-install http "appapi-dsp:2375" "https://nextcloud.local" --set-default --haproxy_password some_secure_password --net nextcloud_default | |
| ./scripts/occ.sh nextcloud -- app_api:daemon:register dsp_http "DSP" docker-install http "appapi-dsp:2375" "http://nextcloud.local" --set-default --haproxy_password some_secure_password --net nextcloud_default |
@kyteinsky I've ran into a few issues when testing an exapp for whitebord with this.
I was thinking we could actually simplify this a bit more. Therefore this draft. Would be curious about your thoughts.
For the DSP itself I don't see a need to have a separate https container. For development purposes the internal communication can just be plain http.
That also saves some hassle generating custom certificates
Not directly linked but possibly related to https://github.com/juliusknorr/nextcloud-docker-dev/pull/382/files#