[JENKINS-31462] GitHub Enterprise Servers validation

[recena]

JENKINS-31462

Please, consider the comments and reviews in this PR #10 (original)

downstream jenkinsci/github-api-plugin#6, downstream hub4j/github-api#232

Result

@reviewbybees, specially @jglick

[ghost]

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.

[oleg-nenashev]

🐝 , but the PR is blocked by github-api

[recena]

@oleg-nenashev Thanks, I'm waiting for hub4j/github-api#232

[recena]

@oleg-nenashev The dependency was upgraded.

[amuniz]

It's no more a TODO

[amuniz]

🐝

[oleg-nenashev]

Occasionally commented in the PR. 🐛 for the loss of the error info in the Web UI validation output

[recena]

@oleg-nenashev

1. I've decided to use API URL because GitHub API Documentation use URL instead of URI
2. I'm showing a helpful message to the use, enough to know that something is wrong

[oleg-nenashev]

I'm removing the bug according to my discussion with @recena
GitHub.connectToEnterpriseAnonymously(api.toString()) may throw IOException (e.g. if GitHub server is unavailable), so I still don't think the current validation is good enough to put a bee.

[oleg-nenashev]

I'm showing a helpful message to the use, enough to know that something is wrong

The message is definitely helpful, but it does not cover all possible cases. Examples of other causes, which may lead to the IOException

• API URL is valid, but GitHub server is unavailable or shut down
• API URL is valid, but GitHub instance rejects anonymous connection attempts

[recena]

@oleg-nenashev

API URL is valid, but GitHub server is unavailable or shut down

It should be considered in the library, not here.

API URL is valid, but GitHub instance rejects anonymous connection attempts

If you mean GitHub Enterprise in private mode, it is considered.

[oleg-nenashev]

But the current code will show "This does not look like a GitHub Enterprise API URL" in both cases, won't it? What prevents you from displaying the exception message in the validation output?

[recena]

@oleg-nenashev I hope you agree with me on both cases are exceptional. The current PR handles:

1. Empty URL
2. Invalid URL (syntax)
3. A valid URL that does not correspond with a GitHub API URL

By the way, I had to modify the code after your first 🐝 because @kohsuke modified my initial proposal. I was confident with the first approach.

[recena]

@oleg-nenashev Trying to improve the PR I've found an example about why don't use e.getMessage() in validation messages when everything is throw in IOException:

Dec 03, 2015 7:09:14 PM org.jenkinsci.plugins.github_branch_source.Endpoint$DesciptorImpl doCheckApiUri
WARNING: Unexpected character ('<' (code 60)): expected a valid value (number, String, array, object, 'true', 'false' or 'null')

Imagine the warning message in the UI.

[recena]

@jglick If you agree, I'd like to merge this PR. IMO is ready to merge.

[oleg-nenashev]

I'm not convinced, but feel free to go forward if you feel strong

[recena]

@oleg-nenashev I totally disagree with show to the user technical information like WARNING: Unexpected character ('<' (code 60)): expected a valid value (number, String, array, object, 'true', 'false' or 'null').

Additionally, all cases that you mentioned before should be handle in the library. I only needed a true or false but @kohsuke changed the method.

[recena]

@oleg-nenashev I agree with show to the user detailed information if he can do anything with this information. Only in these cases.

[amuniz]

You could use catch (MalformedURLException | JsonParseException e) and avoid the duplicated message.

[recena]

I know but we are not using this feature of Java SE 7 in this plugin.

[amuniz]

🐝

[jglick]

🐝

[recena]

@reviewbybees done

[ghost]

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.