Improve checkApiUrlValidity() method #251
Conversation
…itHub Enterprise servers
|
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. |
| try { | ||
| retrieve().to("/", GHApiInfo.class).check(apiUrl); | ||
| } catch (IOException ioe) { | ||
| if (isPrivateModeEnabled()) { |
There was a problem hiding this comment.
I'm not convinced this check is enough. There may be other errors (e.g. network failure), but this check suppresses all other issues. Maybe you should throw a new exception with the cause and a message, which says that it maybe related to the private mode
There was a problem hiding this comment.
Same feeling here, I would prefer a logic based on the HTTP error code (401 for private mode, http proxy auth...) and on the Network exception (connection refused, ssl handshake rejected for negotiation failure...). I am investigating to make proposals.
There was a problem hiding this comment.
The method basically is the same but now we have included a new case.
I tested with:
- https://api.github.com/
- http://github.mycompany.com/api/v3/
- https://github.mycompany.com/api/v3/ (self-signed certificate)
- https://github.mycompany.com/api/v3/ (certificate)
There was a problem hiding this comment.
A Network exception throws a IOException and turns out an negative validation.
There was a problem hiding this comment.
but this check suppresses all other issues
Why? How? I'm only handling a special case. If private mode is not configured and there is an problem, an exception is thrown.
Maybe you should throw a new exception with the cause and a message, which says that it maybe related to the private mode
The exception is thrown with a customized message (IMO, very clear). The doubt can be about the way used to verify the privade mode.
Conflicts: src/main/java/org/kohsuke/github/GitHub.java
Part of JENKINS-33318
@reviewbybees, specially @kohsuke and @cyrille-leclerc