Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JENKINS-31482] Be able to use HTTP/HTTPS for new GH API Endpoints #9

Merged
merged 7 commits into from Nov 10, 2015
Merged

[JENKINS-31482] Be able to use HTTP/HTTPS for new GH API Endpoints #9

merged 7 commits into from Nov 10, 2015
+15 −21

Conversation

recena
Copy link
Contributor

@recena recena commented Nov 9, 2015

JENKINS-31482

This PR is a second part (review) of #6

@reviewbybees

@ghost
Copy link

ghost commented Nov 9, 2015

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.

jglick
jglick reviewed Nov 9, 2015
View reviewed changes
src/main/java/org/jenkinsci/plugins/github_branch_source/AbstractGitHubSCMSource.java
@@ -82,13 +82,14 @@

protected AbstractGitHubSCMSource(String id, String apiUri, String checkoutCredentialsId, String scanCredentialsId, String repoOwner, String repository) {
super(id);
this.apiUri = apiUri;
this.apiUri = Util.fixEmpty(apiUri);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 this directly contradicts the stated intent of the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still contradicts the annotation a few lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@jglick
Copy link
Member

jglick commented Nov 9, 2015

🐛 fixEmpty makes no sense for a field intended to be @Nonnull; and you are missing a readResolve for existing settings using a null (or blank?) field.

@jenkinsadmin
Copy link
Member

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

@recena recena changed the title apiUri is now mandatory, cannot be null Be able to use HTTP or HTTPS for new GH API Endpoints Nov 9, 2015
@recena recena changed the title Be able to use HTTP or HTTPS for new GH API Endpoints Be able to use HTTP/HTTPS for new GH API Endpoints Nov 9, 2015
@recena
Copy link
Contributor Author

recena commented Nov 9, 2015

I forget that the method Connector.connect(String, StandardCredentials) is designed considering apiUri == null then GitHub (SaaS).

amuniz
amuniz reviewed Nov 9, 2015
View reviewed changes
src/main/java/org/jenkinsci/plugins/github_branch_source/RepositoryUriResolver.java
return endpoint.getHost();
} catch (MalformedURLException e) {
// ignore
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, if apiUri is not a well formed URL then github.com is returned. It's a weird behaviour from a UX point of view IMO (if the user sets something wrong then it must be notified back somehow).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This happens when the logic (backend) defines a behavior and the UI other different 😵
The idea is to include input data validation to avoid this kind of checks here.

@recena
Copy link
Contributor Author

recena commented Nov 10, 2015

@jglick I've changed the PoV of this PR. The title has been changed.

@jglick
Copy link
Member

jglick commented Nov 10, 2015

🐝

@jglick
Copy link
Member

jglick commented Nov 10, 2015

If you can verify that #6 introduced the regression I think I saw (http-protocol checkout even though authenticated against github.com), and that this PR corrects it, then I think it qualifies as a priority bug fix eligible for inclusion in 1.0.

@recena
Copy link
Contributor Author

recena commented Nov 10, 2015

@jglick Let me check it.

@recena recena changed the title Be able to use HTTP/HTTPS for new GH API Endpoints [JENKINS-31482] Be able to use HTTP/HTTPS for new GH API Endpoints Nov 10, 2015
@recena
Copy link
Contributor Author

recena commented Nov 10, 2015

@jglick I've tested:

  1. Create a GH Organization using github.com 👍
  2. Create a GH Organization using a GH Enterprise Server (HTTP) 👍

I could not try with GH Enterprise Server (HTTPs) because the certificate generated in the installation process has a different IP (No subject alternative names matching IP address X.X.X.X found).

@recena
Copy link
Contributor Author

recena commented Nov 10, 2015

@jglick I've repeated the same tests using the current master and this result was the result:

  1. Create a GH Organization using github.com 👎 because it uses http to clone
  2. Create a GH Organization using a GH Enterprise Server (HTTP) 👍

@amuniz
Copy link
Member

amuniz commented Nov 10, 2015

🐝

recena added a commit that referenced this pull request Nov 10, 2015
@recena
Merge pull request #9 from recena/reviewOf-JENKINS-31445
04c7597 
[JENKINS-31482] Be able to use HTTP/HTTPS for new GH API Endpoints
@recena recena merged commit 04c7597 into jenkinsci:master Nov 10, 2015
@recena recena deleted the reviewOf-JENKINS-31445 branch November 10, 2015 18:29
@recena
Copy link
Contributor Author

recena commented Nov 10, 2015

@reviewbybees done

@jglick
Copy link
Member

jglick commented Nov 10, 2015

Thanks. I released this as 0.1-beta-4 to assist in testing.

dubrsl pushed a commit to dubrsl/github-branch-source-plugin that referenced this pull request Oct 17, 2020
@baymac
Merge pull request jenkinsci#9 from baymac/remove-synchronized
9062fb7 
[JENKINS-57892] Remove synchronized
dubrsl pushed a commit to dubrsl/github-branch-source-plugin that referenced this pull request Oct 17, 2020
@baymac
Merge pull request jenkinsci#9 from casz/hotfix/reuseFetchOwner
0d12bcd 
[JENKINS-58481] reuse fetchOwner for doCheckProjectOwner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@recena @jglick @jenkinsadmin @amuniz