[JENKINS-33318] GHE server validation with private mode enabled #27

recena · 2016-03-05T22:23:09Z

@reviewbybees

ghost · 2016-03-05T22:25:17Z

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.

jenkinsadmin · 2016-03-06T01:01:40Z

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

cyrille-leclerc · 2016-03-06T17:43:13Z

src/main/java/org/jenkinsci/plugins/github_branch_source/Endpoint.java

            } catch (MalformedURLException mue) {
-                return FormValidation.error("This does not look like a GitHub Enterprise API URL");
+                // For example: https:/api.github.com
+                LOGGER.log(Level.WARNING, "Trying to configure a GitHub Enterprise server: " + apiUri);


I would add the exception in the log call

Agreed. 🐛 for the combination of this + JsonParseException both logging the same error message; each should indicate what is wrong in the log line, and the form validation errors should be different.

Suggestions?

For the warning: "Failed trying to configure a GitHub Enterprise server, malformed URL: " + apiUri

For the form error: "This does not look like a GitHub Enterprise API endpoint (malformed URL)"

Sure. I'll do it now.

cyrille-leclerc · 2016-03-06T17:47:39Z

I added several comments. More broadly, why don't we add the credentials to validate the connection? It seems acceptable to me.

orrc · 2016-03-06T18:35:20Z

I guess this code would work, but I agree with Cyrille and Oleg in the github-api PR that the GitHub server check looks too fragile.

But in any case, as mentioned by myself, Jesse, and Patrick in JENKINS-33228, it does not make sense for this plugin to try and reimplement the existing working functionality from github-plugin.

cyrille-leclerc · 2016-03-06T21:07:34Z

@orrc please note that my comments and PRs do not imply that I favor an "isolated approach" in the github-branch-source-plugin instead of integrating github-branch-source-plugin with github-plugin .

I like the approach of integrating github-branch-source-plugin with github-plugin as you and @KostyaSha have suggested in GitHub server configuration undocumented / duplicated but I also wanted to fix the existing "weakness" waiting for the long term fix.

recena · 2016-03-07T17:34:31Z

@cyrille-leclerc I propose to go forward with this PR. It is not the perfect solution but better than the current validation.

In parallel, I offer to work with @KostyaSha on this proposal.

cyrille-leclerc · 2016-03-07T20:50:55Z

@recena I'm fine with this plan. 👍

jglick · 2016-03-07T21:21:36Z

Error:

Could not resolve dependencies for project org.jenkins-ci.plugins:github-branch-source:hpi:1.4-SNAPSHOT: Could not find artifact org.kohsuke:github-api:jar:1.74-SNAPSHOT in repo.jenkins-ci.org

Do not forget to deploy snapshots.

recena · 2016-03-07T21:39:08Z

@jglick Looking for a way to deploy org.kohsuke:github-api:jar:1.74-SNAPSHOT

recena · 2016-03-07T22:36:48Z

@jglick Deployed.

svanoort · 2016-04-18T13:44:59Z

src/main/java/org/jenkinsci/plugins/github_branch_source/Endpoint.java

+            } catch (FileNotFoundException fnt) {
+                // For example: https://github.mycompany.com/server/api/v3/ gets a FileNotFoundException
+                LOGGER.log(Level.WARNING, "Getting HTTP Error 404 for " + apiUri);
+                return FormValidation.error("This does not look like a GitHub Enterprise API endpoint");


FormValidation error here also could be more explicit.

Suggestions? Here we want only to validate an GitHub API endpoint.

There are three scenarios:

It is a valid URL, for example, https://api.github.com or https://github.mycompany.com/api/v3/ (with private mode disabled).

It is not a valid URL, for example, http://www.google.com, http:/api.github.com, etc...

It is a GitHub Enterprise server in private mode, hence the form validation can not validate.

Yes, I understood that; each one of these error cases should give a different formvalidation error and a different log message, so that the user/Jenkins administrator knows which thing is the problem. For the 404 case, you just need to say: "This does not look like a GitHub Enterprise API endpoint: page not found (error 404)" + apiUrl

In this case I prefer to keep a more generic message because you can get a 404 due to different reasons. Explanation here.

svanoort · 2016-04-18T13:49:31Z

src/main/java/org/jenkinsci/plugins/github_branch_source/Endpoint.java

                LOGGER.log(Level.WARNING, e.getMessage());
-                return FormValidation.error("This does not look like a GitHub Enterprise API URL");
+                return FormValidation.error("This does not look like a GitHub Enterprise API endpoint");


Again: log why it does not look right.

I disagree with using verbose validation messages the UI. The Jenkins log file there will be more detailed info.

It doesn't need to be verbose. Short is fine, but what's the point in showing a validation message in the UI if they don't know why the input is invalid?

@svanoort Addressed e32e694

@recena That commit addresses the log, but what about the form validation? I feel that may actually be more important, since users are more likely to see it than the logs.

@svanoort This was seen in a real environment 😄

@svanoort

This is trivial, and could be done in less time than it takes to reply to one of these comments.

I prefer to close this PR (motivated to improve the current behavior) before to do something I disagree with.

@recena Sorry if I was not clear:

For JsonParseException: FormValidation can get the line "Invalid JSON response" - if you want to, you could also add "(possibly wrong endpoint or server issue?)

For IOException we don't actually do the retry, just suggest the user "verify network and/or try again later" or similar.

As I mentioned before, I disagree with this kind of technical messages. But I'll do it in order to go forward with this PR.

@svanoort Your comments were addressed here. Note I changed the message in order to get sentences shorter.

andresrc · 2016-04-18T14:35:25Z

🐝

recena · 2016-04-18T14:36:56Z

@andresrc Thanks.

andresrc · 2016-04-18T15:31:23Z

🐝

svanoort · 2016-04-18T15:34:37Z

🐝 as of the latest commit. Good to see that we are now returning a reason for validation failure (helpful to user), not just saying it does not look right (not so helpful).

[JENKINS-33318] GHE server validation with private mode enabled

fad71a5

cyrille-leclerc reviewed Mar 6, 2016
View reviewed changes

recena closed this Mar 6, 2016

recena reopened this Mar 7, 2016

recena closed this Mar 7, 2016

recena reopened this Mar 7, 2016

recena closed this Mar 7, 2016

recena reopened this Mar 7, 2016

recena referenced this pull request in hub4j/github-api Mar 14, 2016

This isn't just about API URL but it also checks the valid credential

1954a9f

recena added 2 commits April 18, 2016 15:35

[JENKINS-33318] Reviewed

0748599

[JENKINS-33318] Conflicts with upstream/master

819673c

svanoort reviewed Apr 18, 2016
View reviewed changes

[JENKINS-33318] Added a comment

643c7cd

svanoort reviewed Apr 18, 2016
View reviewed changes

recena added 2 commits April 18, 2016 15:59

[JENKINS-33318] @svanoort's comment was addressed

e32e694

[JENKINS-33318] @svanoort's comment was addressed

6f511f4

recena force-pushed the JENKINS-33318 branch from 0e8f195 to 6f511f4 Compare April 18, 2016 14:16

[JENKINS-33318] Reverted a modified line

c990cdf

[JENKINS-33318] @svanoort's comment was addressed

2fab60d

recena merged commit af04b02 into jenkinsci:master Apr 18, 2016

recena deleted the JENKINS-33318 branch April 18, 2016 15:37

recena mentioned this pull request Apr 18, 2016

Revert "[JENKINS-33318] GHE server validation with private mode enabled" #45

Merged

recena restored the JENKINS-33318 branch April 18, 2016 15:40

recena mentioned this pull request Apr 18, 2016

[JENKINS-33318] GHE server validation with private mode enabled #46

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-33318] GHE server validation with private mode enabled #27

[JENKINS-33318] GHE server validation with private mode enabled #27

recena commented Mar 5, 2016 •

edited

Loading

ghost commented Mar 5, 2016

jenkinsadmin commented Mar 6, 2016

cyrille-leclerc Mar 6, 2016

svanoort Apr 18, 2016

recena Apr 18, 2016

svanoort Apr 18, 2016

recena Apr 18, 2016

cyrille-leclerc commented Mar 6, 2016

orrc commented Mar 6, 2016

cyrille-leclerc commented Mar 6, 2016

recena commented Mar 7, 2016

cyrille-leclerc commented Mar 7, 2016

jglick commented Mar 7, 2016

recena commented Mar 7, 2016

recena commented Mar 7, 2016

svanoort Apr 18, 2016

recena Apr 18, 2016

recena Apr 18, 2016 •

edited

Loading

svanoort Apr 18, 2016

recena Apr 18, 2016

svanoort Apr 18, 2016

recena Apr 18, 2016

svanoort Apr 18, 2016

recena Apr 18, 2016

svanoort Apr 18, 2016

recena Apr 18, 2016

recena Apr 18, 2016

svanoort Apr 18, 2016

recena Apr 18, 2016

recena Apr 18, 2016

andresrc commented Apr 18, 2016

recena commented Apr 18, 2016

andresrc commented Apr 18, 2016

svanoort commented Apr 18, 2016

[JENKINS-33318] GHE server validation with private mode enabled #27

[JENKINS-33318] GHE server validation with private mode enabled #27

Conversation

recena commented Mar 5, 2016 • edited Loading

ghost commented Mar 5, 2016

jenkinsadmin commented Mar 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyrille-leclerc commented Mar 6, 2016

orrc commented Mar 6, 2016

cyrille-leclerc commented Mar 6, 2016

recena commented Mar 7, 2016

cyrille-leclerc commented Mar 7, 2016

jglick commented Mar 7, 2016

recena commented Mar 7, 2016

recena commented Mar 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recena Apr 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresrc commented Apr 18, 2016

recena commented Apr 18, 2016

andresrc commented Apr 18, 2016

svanoort commented Apr 18, 2016

recena commented Mar 5, 2016 •

edited

Loading

recena Apr 18, 2016 •

edited

Loading