unison icon indicating copy to clipboard operation
unison copied to clipboard

refactor: show more info about connection errors in output

Open mitchellwrosen opened this issue 1 year ago • 4 comments

Overview

Addresses #4619; I thought it might be find to just print out the whole error for the user, rather than hide it behind a friendlier but less informative "something went wrong".

However, I now see in the bug report that "we don't want to dump out cryptic errors normally". So, this might need more work. Nonetheless I thought I'd throw it up for consideration.

mitchellwrosen avatar Jan 16 '24 16:01 mitchellwrosen

Thanks @mitchellwrosen! I know that it's different than what @aryairani said, but my personal opinion is that for a tool that targets developers a verbose error message is better than failure without any indication of what's going wrong.

Minor nitpick: I don't know what all errors can be represented by these two constructors, but I think that some (if not all) of them happen when you can't actually connect to the server so saying that you got an error from the server might not quite be accurate.

ceedubs avatar Jan 16 '24 18:01 ceedubs

Thanks @mitchellwrosen; I will get a second opinion about the cryptic errors. @ceedubs Can you use this draft PR build to answer your question in the mean time? Or no, you need it to be in a release to get it into your sandbox?

I guess we don't know exactly how to trigger the errors to even know how cryptic they will be, right?

aryairani avatar Jan 17 '24 03:01 aryairani

I'm attempting to try from this branch and am waiting on a lengthy build. I'll leave an update when it finishes.

ceedubs avatar Jan 17 '24 15:01 ceedubs

Worked great! The problem was one on my end and resulted in the following error message:

   tmp/main> pull @unison/httpserver/releases/3.0.2 lib.httpserver_3_0_2

      Oops, I encountered an unexpected exception from the server.

        InternalException
            ( HandshakeFailed
                ( Error_Protocol
                    ( "certificate has unknown CA"
                    , True
                    , UnknownCa
                    )
                )
            )

ceedubs avatar Jan 17 '24 17:01 ceedubs

What should we do with this one @aryairani?

mitchellwrosen avatar May 24 '24 15:05 mitchellwrosen

@mitchellwrosen Let's special-case Cody's error, which will serve as scaffolding for special-cased errors. If we miss the special cases, we fall through to the current output.

For reference, what library does that error come from? We can take a look at some of the possible error values and preemptively construct some nicer error messages.

For example, match on UnknownCa and the True if it's relevant, and print

I couldn't perform the pull because the server's SSL certificate has an unknown CA.

instead of the existing output.

aryairani avatar Jun 01 '24 14:06 aryairani