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.

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

use http(s)/ssh URL by API's result rathar than fixed pattern. #63

Closed
wants to merge 1 commit into from

Conversation

kounoike
Copy link

GitBucket is an opensource Git platform with high extensibility & github API compatibility.
It act as GitHub (Enterprise). But some differences are exists.

In GitBucket's side problems are fixed by my PR. gitbucket/gitbucket#1247, gitbucket/gitbucket#1248
But last one problem can't solve by GitBucket side.

PROBLEM
GitBucket provides git repository such as: http://server:port/git/user/repo.git
GitHub Branch Source plugin use fixed pattern URL such as: http://server/user/repo.git

SOLUTION
GitHub and GitBucket API returns http(s)/ssh git repository URL. Clients should use this URL rathar than fixed pattern URL.

@kounoike
Copy link
Author

I can't understand for this Jenkins check.

  • What problem occurs?
  • What line of code has problem?
  • How to fix it?

@jglick jglick closed this Sep 19, 2016
@jglick jglick reopened this Sep 19, 2016
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Not mergeable as is. Probably something like this could be merged. Would need to spend some time creating a GitBucket test harness to validate changes.

@@ -327,7 +333,8 @@ public void setBuildForkPRHead(boolean buildForkPRHead) {

@Override
public String getRemote() {
return getUriResolver().getRepositoryUri(apiUri, repoOwner, repository);
String credentialsId = getCredentialsId();
Copy link
Member

Choose a reason for hiding this comment

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

Why?

@@ -134,13 +138,15 @@
private @Nonnull Boolean buildForkPRHead = DescriptorImpl.defaultBuildForkPRHead;

@DataBoundConstructor
public GitHubSCMSource(String id, String apiUri, String checkoutCredentialsId, String scanCredentialsId, String repoOwner, String repository) {
public GitHubSCMSource(String id, String apiUri, String checkoutCredentialsId, String scanCredentialsId, String repoOwner, String repository, String httpGitUrl, String sshGitUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

Impossible to configure this on a multibranch project without corresponding getters and form fields.

super(id);
this.apiUri = Util.fixEmpty(apiUri);
this.repoOwner = repoOwner;
this.repository = repository;
this.scanCredentialsId = Util.fixEmpty(scanCredentialsId);
this.checkoutCredentialsId = checkoutCredentialsId;
this.httpGitUrl = Util.fixEmpty(httpGitUrl);
this.sshGitUrl = Util.fixEmpty(sshGitUrl);
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this should be fixed by inspecting the apiUri instead.

@jglick
Copy link
Member

jglick commented Sep 19, 2016

FindBugs notes:

Dead store to credentialsId in org.jenkinsci.plugins.github_branch_source.GitHubSCMSource.getRemote() [org.jenkinsci.plugins.github_branch_source.GitHubSCMSource] At GitHubSCMSource.java:[line 336] DLS_DEAD_LOCAL_STORE

@stephenc
Copy link
Member

I think a better plan would be a GitBucket specific implementation. The rate limit complexity in this code base is not likely something that a GitBucket plugin will ever have to deal with, additionally users will not realise that there is a GitBucket plugin for Jenkins unless there is an actual GitBucket plugin for Jenkins.

I would recommend using https://github.com/jenkinsci/gitea-plugin as a reference for a nice clean implementation of a GitBucket plugin.

In any case, if it is easy to fix the GitHub plugin to use the urls rather than generate them, then that would probably be a change I would like to merge (provided it does not compromise functionality when there are network connectivity issues)

If you would like to continue with this change, the codebase should be relatively stable now. A simple ping back to let us know you intend to work on this would be nice.

If you do not intend to pick this up in the next 2-3 months, please close the PR.

(Note to self, close this PR if no comments after 2017-Aug-3)

@bitwiseman bitwiseman closed this Apr 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants