toolshed/abra
toolshed/abra
4
2
Fork 18
Code Issues 29 Pull Requests 3 Packages Releases 40 Activity

fix #507 #509

Closed
Apfelwurm wants to merge 1 commits from Apfelwurm/abra:own/fix_addserver into main
pull from: Apfelwurm/abra:own/fix_addserver
merge into: toolshed:main
Conversation 5 Commits 1 Files Changed 3 +9 -3
Apfelwurm commented 2025-03-07 16:06:29 +00:00
Member
Copy Link

This fixes the error output when using abra add server, so it actually returns the relevant exception message. Created with the help of @P4u1

And since i did not write anything on my last PR:
Hey all, i'm Alex and i'm one of the members of Klasse & Methode :)

This fixes the error output when using abra add server, so it actually returns the relevant exception message. Created with the help of @P4u1 And since i did not write anything on my last PR: Hey all, i'm Alex and i'm one of the members of Klasse & Methode :)
🎉 2
Apfelwurm added 1 commit 2025-03-07 16:06:30 +00:00
fix #507
All checks were successful
continuous-integration/drone/pr Build is passing
Details
be9f613112
decentral1se reviewed 2025-03-10 13:00:22 +00:00
decentral1se left a comment
Owner
Copy Link

Great stuff, thanks @Apfelwurm 🎉

Great stuff, thanks @Apfelwurm 🎉
pkg/client/client.go
@ -89,7 +89,7 @@ func New(serverName string, opts ...Opt) (*client.Client, error) {
info, err := cl.Info(context.Background())
if err != nil {
return cl, sshPkg.Fatal(serverName, err)
decentral1se commented 2025-03-10 12:59:50 +00:00
Owner
Copy Link

Is sshPkg.Fatal used anywhere else and/or should it be removed?

Is `sshPkg.Fatal` used anywhere else and/or should it be removed?
Apfelwurm commented 2025-03-10 18:00:37 +00:00
Author
Member
Copy Link

It is still also in use in https://git.coopcloud.tech/toolshed/abra/src/branch/main/cli/server/add.go#L106 :)

It is still also in use in https://git.coopcloud.tech/toolshed/abra/src/branch/main/cli/server/add.go#L106 :)
👍 1
decentral1se marked this conversation as resolved
decentral1se commented 2025-03-11 09:17:34 +00:00
Owner
Copy Link

Sorry, to look at this again, is it not the problem that repeatedly saying "try ssh'ing to the server" is confusing when that does indeed work #507 (comment)

What sshPkg.Fatal is trying to do is "route" error messages so that what is given back to the user is very specific and is
not a "wall of text"

So, for me, the ideal situation would be to handle "Unknown command: docker" (see #507 logs) with a specific error messages, aka "are you sure Docker is installed / your user has the permissions"?

We could also additionally match for "please make sure the URL is valid, and Docker 18.09 or later is installed on the remote host" to give a similar warning.

sshPkg.Fatal was implemented specifically to help people get better feedback based on many issues raised over the years on this connection situation, which often effects beginners disproportionately.

I would normally be up for merging stuff and seeing how it goes but I've noticed that changes around error message handling which is given directly to users needs to be handled very carefully or can result in $more issue reports and growing frustration.

Let me know what you think @Apfelwurm and thanks again!

Sorry, to look at this again, is it not the problem that repeatedly saying "try ssh'ing to the server" is confusing when that does indeed work https://git.coopcloud.tech/toolshed/abra/issues/507#issuecomment-22955 What `sshPkg.Fatal` is trying to do is "route" error messages so that what is given back to the user is very specific and is not a "wall of text" So, for me, the ideal situation would be to handle "Unknown command: docker" (see #507 logs) with a specific error messages, aka "are you sure Docker is installed / your user has the permissions"? We could also additionally match for "please make sure the URL is valid, and Docker 18.09 or later is installed on the remote host" to give a similar warning. `sshPkg.Fatal` was implemented specifically to help people get better feedback based on many issues raised over the years on this connection situation, which often effects beginners disproportionately. I would normally be up for merging stuff and seeing how it goes but I've noticed that changes around error message handling which is given directly to users needs to be handled very carefully or can result in `$more` issue reports and growing frustration. Let me know what you think @Apfelwurm and thanks again!
p4u1 closed this pull request 2025-03-12 16:17:54 +00:00
p4u1 reopened this pull request 2025-03-12 16:17:58 +00:00
p4u1 commented 2025-03-13 08:05:05 +00:00
Member
Copy Link

@decentral1se I think this pull request is doing what you wrote in you last message

To improve error handling in general I create a new pr with a proof of concept for errors with help messages: #512

@decentral1se I think this pull request is doing what you wrote in you last message To improve error handling in general I create a new pr with a proof of concept for errors with help messages: https://git.coopcloud.tech/toolshed/abra/pulls/512
❤️ 1
decentral1se added this to the abra v0.10.0 project 2025-03-16 11:29:25 +00:00
decentral1se commented 2025-03-16 17:50:00 +00:00
Owner
Copy Link

@p4u1 @Apfelwurm

@decentral1se I think this pull request is doing what you wrote in you last message

Just to share what was discussed elsewhere:

image
@p4u1 @Apfelwurm > @decentral1se I think this pull request is doing what you wrote in you last message Just to share what was discussed elsewhere: <img width="431" alt="image" src="attachments/54fd271e-91b6-4cfc-be6c-8b28adda2afc">
image.png
37 KiB
decentral1se commented 2025-03-22 12:30:45 +00:00
Owner
Copy Link

@p4u1 and myself agreed that I can come up with an adjusted fix for this.

Coming Soon ™

@p4u1 and myself agreed that I can come up with an adjusted fix for this. Coming Soon ™
❤️ 1
decentral1se closed this pull request 2025-03-22 12:30:45 +00:00
decentral1se moved this to Done in abra v0.10.0 on 2025-04-14 22:00:59 +00:00
decentral1se moved this to Done in abra v0.10.0 on 2025-04-14 22:01:01 +00:00
decentral1se moved this to Done in abra v0.10.0 on 2025-04-16 05:16:08 +00:00
decentral1se moved this to Done in abra v0.10.0 on 2025-04-16 05:16:10 +00:00
decentral1se moved this to Done in abra v0.10.0 on 2025-04-19 07:28:29 +00:00
decentral1se moved this to Done in abra v0.10.0 on 2025-04-21 17:48:16 +00:00
All checks were successful
continuous-integration/drone/pr Build is passing
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something is not working
build
go build related issues
ci/cd
Building things with CI/CD
contributing
Dealing with contributing
critical fix
https://docs.coopcloud.tech/federation/resolutions/passed/010/
design
UI/UX
documentation
Documenting all the things
duplicate
This issue or pull request already exists
enhancement
New feature
help wanted
Need some help
invalid
Something is wrong
meta
relating to the project or repo itself
question
More information is needed
release
Release management
release-candidate
Related to the new release candidate
security
Security related
test
Unit/integration testing
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project abra v0.10.0
Assignees
Clear assignees
3wordchant abra-bot ammaratef45 Apfelwurm carla cas decentral1se fauno kawaiipunk knoflook lambdabundesverband moritz p4u1 simon trav val yksflip
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: toolshed/abra#509
No description provided.
Delete Branch "Apfelwurm/abra:own/fix_addserver"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?