Add defensive checks to prevent inscrutable error messages #4913
Conversation
| # How to implement this though? | ||
| # * Ruby has no built-in URL parsing, and no great alternatives in https://stackoverflow.com/q/1805761/770425... | ||
| # Not sure what Dependabot team policy is on using 3rd-party gems? | ||
| # Alternatively a basic sanity check of "it should not contain whitespace" may suffice for now... ?? |
There was a problem hiding this comment.
There was a problem hiding this comment.
Do we expect a URL here or just the module path? If it's the path we could use some of the basic character checks from https://pkg.go.dev/golang.org/x/mod/module#CheckImportPath / https://pkg.go.dev/golang.org/x/mod/module#CheckPath as a start? Agree we just need a simple defensive check rather than a robust validation here.
| # TODO: currently this matches last. Instead, if more than one match, and they | ||
| # aren't identical, then don't try to be clever with GitDependenciesNotReachable | ||
| # but instead raise DependencyFileNotResolvable and report the whole error. | ||
| # This would have resulted in a more obvious error message for #4625 |
There was a problem hiding this comment.
I actually wasn't 100% sure on this... I could see a case where a multi-line error string may have multiple URLs, and the last one is indeed the proper one to match on...
But given that GitDependenciesNotReachable tells the user that Dependabot can't reach the repo and to update their access creds, that's a fairly opinionated solution...
IMO I'd only want to report that if 110% sure that's the issue, otherwise surface the whole error message and let the user diagnose the issue.
(I assume DependencyFileNotResolvable reports the message to the end user... if it just hides it in logs that only Dependabot devs can see, then that's not so helpful either.)
There was a problem hiding this comment.
I see these cases as most useful to raise GitDependenciesNotReachable.
- You have a direct dependency on a private github repo but Dependabot doesn't have access to it.
- You have an indirect dependency on a github private repo (from another direct private github dependency) but Dependabot doesn't have access to it.
Do we have examples of what multiple URLs look like in this error message handy? I imagine this could get tricky if you had a failing indirect dependency on another host that came from one of your direct github dependencies.
jeffwidman
force-pushed
the
add-defensive-checks-to-prevent-inscrutable-error-messages
branch
from
139aaaf to
c762fc2
Compare
Part of the reason dependabot#4625 was so difficult to debug was the original error messages were a total red herring. They appeared to be related to IPs, but digging deeper realized they were actually due to errors happening earlier in the processing pipeline but not caught until several steps down the line. So this adds some defensive checks so that if there's an upstream error, we catch it sooner and report the proper error message(s).
jeffwidman
force-pushed
the
add-defensive-checks-to-prevent-inscrutable-error-messages
branch
from
c762fc2 to
74f3942
Compare
Part of the reason
#4625 was so
difficult to debug was the original error messages were a total red
herring. They appeared to be related to IPs, but digging deeper realized
they were actually due to errors happening earlier in the processing
pipeline but not caught until several steps down the line.
So this adds some defensive checks so that if there's an upstream error,
we catch it sooner and report the proper error message(s).