Better logs and error messages #25
Conversation
|
@reviewbybees |
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
🐝 |
|
Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests. |
|
@cyrille-leclerc Before to merge this one, I'm reviewing this #23 it is related. |
|
DON NOT MERGE! |
| return; | ||
| } | ||
|
|
||
| if (!github.isAnonymous()) { | ||
| listener.getLogger().format("Connecting to GitHub using %s%n", CredentialsNameProvider.name(credentials)); | ||
| listener.getLogger().format("Connecting to %s using %s%n", apiUri == null ? "github.com" : apiUri, CredentialsNameProvider.name(credentials)); |
There was a problem hiding this comment.
Use HttpsRepositoryUriResolver.hostnameFromApiUri()
There was a problem hiding this comment.
@recena RepositoryUriResolver#hostnameFromApiUri just returns the hostname when I would like to display the entire API URL. I suspect that some people will make a mistake defining the API URL and will be happy to see the entire URL.
BTW I did not catch why GitHubSCMNavigator#apiUri is nullable with a kind of discovery happening latter on.
There was a problem hiding this comment.
@cyrille-leclerc This only makes sense when you are working with a GHE instance. When you configure an instance, its endpoint has always the same format: http(s)://hostname/api/v3/. For that reason, the important part is the hostname.
|
🐜 because there is an explicit method to resolve the hostname from an apiUrl. |
|
I would prefer to have the entire API URL including port, URI & protocol rather than just the hostname.
|
|
@cyrille-leclerc It should not happen if the form validation for GitHub Enterprise instances is working fine. |
|
@cyrille-leclerc I don't want to block this PR but my opinion is on the table. |
|
🐝 |
|
@reviewbybees done |
…sages Better logs and error messages
I wish you were with me when I tried to understand why things went wrong in my setup. I propose that we lower the level of details of the logs once the code stops silently ignoring critical exception like FYI even org.kohsuke:github-api silently swallows IOException. I'll show you how hard it is to findout that an SSL cert is not trusted. Places where we silently swallow exceptions:
|
|
@recena thanks for merging! I shared my opinion too :-) |
|
@cyrille-leclerc I agree with the motivation of your PR, I only recommend to avoid a lot of ternary operators and magic strings like If you want we can discuss all these cases when you file the corresponding bugs. |
|
Generally any code which looks like } catch (IOException e) {
// ignore
}is wrong. You should be logging (perhaps at |
Agreed. |
Generally is a way to say in the majority cases but when you use GitHub API you will find exceptions. |
|
@recena spend few months at support and then you"ll change your mind about silently ignoring API errors. |
|
@ndeloof Thanks for your advice I never thought about it. |
|
I'm 100% with you @cyrille-leclerc — I tried using this plugin and got zero feedback about why my config wasn't working, or why my GH Enterprise URL was "wrong". Thanks for the improvements! |
|
@orrc Did you file some issue? When did you set the GitHub Enterprise instance the form validation was supported? It was included in |
Better logs and error messages. Especially for SSL and invalid credentials.