toolshed/abra
4
4
Fork 23
Code Issues 82 Pull Requests 12 Packages Releases 45 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: :main
Conversation 5 Commits 1 Files Changed 3
+9 -3
Apfelwurm commented 2025-03-07 16:06:29 +00:00
Member
Copy Link
Copy Source

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
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
Copy Source

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
Copy Source

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
Copy Source

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
Copy Source

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
Owner
Copy Link
Copy Source

@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
Copy Source

@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
Copy Source

@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

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
decentral1se
Labels
Clear labels
bug
Something is not working
 build
go build related issues
 ci/cd
Building things with CI/CD
 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
 easy-first-issue
Something for new people to get stuck into. We hope it's easy!
 enhancement
New feature
 help wanted
Need some help
 i10n
Everything to do with localisation
 i18n
Everything to do with internationalisation
 installer
Everything to do with the install script.
 invalid
Something is wrong
 question
More information is needed
 release
Release management
 release-candidate
Related to the new release candidate
 security
Security related
 tech-debt
test
Unit/integration testing
 wontfix
This won't be fixed
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
abra v0.10.0
Done
Assignees
Clear assignees
3wordchant aadil (Aadil Ayub) abra-bot (Abra Bot) ammaratef45 amras (Sarma) Apfelwurm BornDeleuze Brooke carla cas (Cassowary) coopcloud cyrnel decentral1se (d1) dede devydave fauno (fauno) iexos jade (Jade Ambrose) jjsfunhouse jmakdah2 (Jackie Makdah) joe-irving (Joe Irving) kawaiipunk (KawaiiPunk) knoflook kolaente lambdabundesverband linnealovespie (April) moosemower moritz notplants oxaliq (sorrel) p4u1 pharaohgraphy (Andrew 🐦‍🔥❤️‍🔥✴️) renovate-bot (Comrade Renovate Bot) ripclap simon sixsmith (Sixsmith) stevensting trav (Trav Fryer) val (val (he/him)) yksflip
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: toolshed/abra#509