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

Improve error logging #510

Merged
merged 1 commit into from
Feb 13, 2025
Merged

Improve error logging #510

merged 1 commit into from
Feb 13, 2025

Conversation

R0Wi
Copy link
Member

@R0Wi R0Wi commented Feb 12, 2025

  • Check local socket by assuming it starts with a slash
  • Improve error logging for ping and deploy

Addresses #498

Log messages now look like this when doint a ping via UI (if either the docker socket is not writable or the file path doesn't exist)

socket doesn't exist:
socket-does-not-exist

socket is not writable:
socket-not-writable

Also the frontend dialog for the deployment now shows

deployment error for local socket:
deploy-result-err

deployment error for nextcloud DSP:
deploy-result-err-dsp

Unfortunately I didn't find a way to get rid of the slightly misleading error message that - in case the socket is not usable - curl will always complain about not being able to connect tho http://localhost/, even though it's not using any http under the hood but trying to access the UNIX socket directly. When using curl in socket mode ( CURLOPT_UNIX_SOCKET_PATH), it will still need some http-address for the actual GET call. See also curl/curl#1338

* Check local socket by assuming it starts with a slash
* Improve error logging for ping and deploy

Signed-off-by: Robin Windey <[email protected]>
@R0Wi R0Wi requested a review from oleksandr-nc February 12, 2025 20:03
@oleksandr-nc
Copy link
Contributor

@kyteinsky or @andrey18106 - second review?

note: we will backport this to the NC31/NC30

Copy link
Collaborator

@andrey18106 andrey18106 left a comment

Choose a reason for hiding this comment

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

LGTM

@R0Wi R0Wi enabled auto-merge February 13, 2025 16:45
@R0Wi R0Wi merged commit 12ee1b7 into main Feb 13, 2025
32 checks passed
@R0Wi R0Wi deleted the fix#498 branch February 13, 2025 16:46
@R0Wi
Copy link
Member Author

R0Wi commented Feb 13, 2025

/backport to stable31

@R0Wi
Copy link
Member Author

R0Wi commented Feb 13, 2025

/backport to stable30

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.

4 participants