Extension: RepositorySizeGithubAPI to get size of a repo #316
Conversation
This class intends to implement an extension point provided by the Git Plugin which should return the size of a repository if the provided URL is applicable to the Github Branch Source Plugin.
|
@bitwiseman This PR will be blocked by PR-931.
|
| * This extension intends to perform a GET request without any credentials on the provided repository URL | ||
| * to return the size of repository. | ||
| */ |
There was a problem hiding this comment.
Git Plugin is adding a new class called the GitToolChooser which requires the size of a remote repository to recommend an optimal git implementation ("git" or "jgit").
This size can be measured in two ways:
- By cache created by a multi branch project
- From REST APIs exposed by the git providers themselves
Since the git plugin probably doesn't involve calling any REST API in the code base, we would like to delegate the task to the plugins which do.
In terms of the GitHub Branch Source Plugin, implementing this extension serves no purpose for the plugin, it only acts as a helper method for git plugin to improve internal performance.
|
@bitwiseman Is there anything I could do expedite the review process? Would you like to see the automated test cases first? |
|
|
||
| @Override | ||
| public boolean isApplicableTo(String repoUrl) { | ||
| return repoUrl.contains("github"); |
There was a problem hiding this comment.
This is not sufficient. GitHub Enterprise servers can have any kind of name with our without "github" in the url. Also, there could be git repos with "github" in the name.
There was a problem hiding this comment.
Okay, I understand. So this means checking the repository url might not be enough to confirm the applicability. Would you suggest that we send some user info as well? A combination of those two could be enough to see if it is being provided by Github?
|
@bitwiseman Noted, the only reason for me to put it in |
…repo on the basis of the repostitory url and the user credentials provided to it.
|
@bitwiseman I have added better checks in I am having some issues in writing automated test cases for this extension though, particularly because of the reason that the extension contacts the Github REST API. Could you provide some pointers on how to write a test case for this? I have created a test class, please review it and any feedback will be appreciated! Thanks. |
…udhouliya/github-branch-source-plugin into add-gitplugin-extension
| // WorkflowJob p = j.jenkins.createProject(WorkflowJob.class, "p"); | ||
| // p.setDefinition(new CpsFlowDefinition( | ||
| // "node {\n" | ||
| // + " checkout(\n" | ||
| // + " [$class: 'GitSCM', \n" | ||
| // + " userRemoteConfigs: [[credentialsId: 'github', url: $/" + url + "/$]]]\n" | ||
| // + " )" | ||
| // + "}", true)); | ||
| // WorkflowRun b = j.assertBuildStatusSuccess(p.scheduleBuild2(0)); | ||
| // j.waitForMessage("using credential github", b); |
There was a problem hiding this comment.
There are a couple of ways to do this: use Mockito to directly mock the calls to GitHub and/or use Wiremock to create a local web server that simulates. the responses from GitHub.
Example: adddd68
There was a problem hiding this comment.
Thanks for providing an example,
I've been using Mockito to mock the calls to Github but I'm stuck at mocking static methods which is not possible with Mockito.
Reference: Connector.connect method is a static method provided to return a GitHub object.
|
|
||
| assertThat(repositorySizeGithubAPI.isApplicableTo(url, items.get(0), "github"), is(true)); | ||
| assertThat(api.isApplicableTo("https://github.com/rishabhBudhouliya/zoom-suspender.git", context, "github"), is(false)); |
There was a problem hiding this comment.
@bitwiseman The GitHub.checkApiUrlValidity() is reaching for https://api.github.com/ and since the credentials are not valid, it returns a bad credentials error.
To write the test cases, I would need to wiremock this and I have tried doing that looking at the test case examples provided by you but I have been unsuccessful in doing so.
This is the first step
|
While running this patch, it logged the following exception: I think that ssh repository URLs are common enough that the code should not log a stack trace for each reference to the repository URL. Might make sense to attempt a transformation from ssh format to https format, though my work on jenkinsci/git-plugin#947 has shown me that can be complicated when we're trying to work with multiple SCM providers (like Bitbucket). |
|
An instance of the illegal argument exception blocks my attempt to clone an Assembla repository as well. The stack trace in the job is: |
|
@MarkEWaite Apologies for not addressing this sooner, the reason we didn't see this error while I was testing an updated version of this plugin is that some of the local changes were not committed to this PR. I realised that I've added those changes in the latest commits, now I hope that you won't see these issues while running this patch. |
@MarkEWaite That is an interesting suggestion, we could definitely try that. In the current implementation, we simply reject any git protocol (other than HTTPS). We could take a similar approach for individual extensions. |
…udhouliya/github-branch-source-plugin into add-gitplugin-extension
Git plugin 4.4.0 requires Jenkins 2.204.1
RepositorySizeGithubAPI
This class intends to implement an extension point provided by the Git Plugin
which should return the size of a repository if the provided URL is applicable
to the Github Branch Source Plugin.
Please refer to jenkinsci/git-plugin#931 for further description on this extension and why does the Git Plugin needs it.
The extension point in the Git Plugin: https://github.com/jenkinsci/git-plugin/blob/6dfacf73ea73f94edbedd83fc09336a03ac15659/src/main/java/jenkins/plugins/git/GitToolChooser.java#L169-L178
Note: Since this extension would require a dependency from a git plugin class which hasn't been released yet, I believe we can't merge this PR until PR-931 is merged.
Submitter checklist
Reviewer checklist
Documentation changes
Users/aliases to notify