improve auth error on clone

[dtrifiro]

• exceptions: update AuthError message
• dulwich: add AuthError on HTTPUnauthorized

fixes treeverse/dvc#8654

[codecov-commenter]

Codecov Report

Base: 81.66% // Head: 81.63% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (c375ec6) compared to base (52b5b4f).
Patch coverage: 36.36% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #159      +/-   ##
==========================================
- Coverage   81.66%   81.63%   -0.04%     
==========================================
  Files          25       25              
  Lines        3404     3408       +4     
  Branches      588      588              
==========================================
+ Hits         2780     2782       +2     
- Misses        542      544       +2     
  Partials       82       82              

Impacted Files	Coverage Δ
src/scmrepo/exceptions.py	72.00% <0.00%> (ø)
src/scmrepo/git/backend/dulwich/__init__.py	79.13% <0.00%> (ø)
src/scmrepo/git/backend/dulwich/asyncssh_vendor.py	83.33% <66.66%> (-1.81%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[dberenbaum]

Minor: Can we also include the exception name (dulwich.client.HTTPUnauthorized)? I think it's helpful to see where it originated. It can be hard to debug because we are wrapping other exceptions with our own exceptions.

Less minor: Why is this specific to HTTPUnauthorized? Can we make CloneError always show the original exception class and message? Is there a reason we don't want to?

[dtrifiro]

Minor: Can we also include the exception name (dulwich.client.HTTPUnauthorized)? I think it's helpful to see where it originated. It can be hard to debug because we are wrapping other exceptions with our own exceptions.

raise AuthError from exc will show the dulwich exception in the traceback (although only when used with -v).

I think the AuthError with the Authentication failed message makes it a bit clearer, and extra information can always be retrieved with -v.

If you think the dulwich exception is clearer, we could also just stop catching HTTPUnauthorized so that it is re-raised as it is (in clone, iter_remote_refs and push_refspecs, which are related to experiments)

[dberenbaum]

Thanks @dtrifiro!

If you think the dulwich exception is clearer, we could also just stop catching HTTPUnauthorized so that it is re-raised as it is (in clone, iter_remote_refs and push_refspecs, which are related to experiments)

I think it's clearer to show Failed to clone repo... in addition to some authentication error. I'm not sure it's obvious to all users that dvc is doing a git clone, so I'd rather raise the CloneError along with the info from the original exception.

More importantly, I'd rather do this for other exceptions so that any other clone failures can be more easily debugged. I know it's maybe duplicating the point of -v, but I think it's generally useful enough to be worth including and saving people from needing -v in some cases.

For example, instead of:

ERROR: failed to get 'vimrc' from 'git@github.com:..' - Failed to clone repo 'git@github.com:...' to '/var/folders/...'

It would be more helpful to immediately see:

ERROR: failed to get 'vimrc' from 'git@github.com:..'
Failed to clone repo 'git@github.com:...' to '/var/folders/...'
Caused by: FileNotFoundError: [Errno 2] No such file or directory: '/Users/dave/.ssh/id_ed25519'

One more minor thing - the AuthError still shows as an 'unexpected error', unlike the CloneError, although I don't know why.

[dtrifiro]

How about something like this:

ERROR: failed to get 'README.md' from 'https://github.com/dberenbaum/example-get-started.git' - Failed to clone repo 'https://github.com/dberenbaum/example-get-started-cp.git' to '/tmp/tmpmxbwwvwmdvc-clone' - No valid credentials provided

[dberenbaum]

How about something like this:

The error message looks good, thanks!

More importantly, I'd rather do this for other exceptions so that any other clone failures can be more easily debugged.

What do you think about this?

[skshetry]

dvc is responsible for error messages, not the scmrepo. Exception should hold enough information to generate in dvc.

[skshetry]

For the clone, since we already have CloneError, it might be better to reuse that and only change the exception message.

[dtrifiro]

Yep, that's what I was thinking

[skshetry]

One more minor thing - the AuthError still shows as an 'unexpected error', unlike the CloneError, although I don't know why.

CloneError is handled in dvc and reraised as dvc's error.