github-branch-source-plugin icon indicating copy to clipboard operation
github-branch-source-plugin copied to clipboard

Extension: RepositorySizeGithubAPI to get size of a repo

Open rishabhBudhouliya opened this issue 5 years ago • 8 comments
trafficstars

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 https://github.com/jenkinsci/git-plugin/pull/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

  • [ ] Link to JIRA ticket in description, if appropriate.
  • [ ] Change is code complete and matches issue description
  • [ ] Automated tests have been added to exercise the changes
  • [ ] Reviewer's manual test instructions provided in PR description. See Reviewer's first task below.

Reviewer checklist

  • [ ] Run the changes and verify that the change matches the issue description
  • [ ] Reviewed the code
  • [ ] Verified that the appropriate tests have been written or valid explanation given

Documentation changes

  • [ ] Link to jenkins.io PR, or an explanation for why no doc changes are needed

Users/aliases to notify

rishabhBudhouliya avatar Jul 28 '20 22:07 rishabhBudhouliya

@bitwiseman This PR will be blocked by PR-931. If you could review the code of the extension w.r.t:

  • Is GitHubSCMSource the right place to add this extension?
  • the isApplicableTo method is used by the Git Plugin to delegate the task of validating the given url as the correct url for the plugin which chooses to implement the extension point. Currently, it just checks if it contains a string "Github". I would assume we would want to add a check which validates the https connection with the provided url.

rishabhBudhouliya avatar Jul 28 '20 22:07 rishabhBudhouliya

@bitwiseman Is there anything I could do expedite the review process? Would you like to see the automated test cases first?

rishabhBudhouliya avatar Aug 03 '20 18:08 rishabhBudhouliya

@bitwiseman Noted, the only reason for me to put it in GitHubSCMSource was the ease of finding the right APIs to create a connection, but I suppose that can be done from a separate class as well.

rishabhBudhouliya avatar Aug 05 '20 13:08 rishabhBudhouliya

@bitwiseman I have added better checks in isApplicableTo method and have shifted the class from GitHubSCMSource.

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.

rishabhBudhouliya avatar Aug 10 '20 13:08 rishabhBudhouliya

While running this patch, it logged the following exception:

java.net.MalformedURLException: no protocol: [email protected]:MarkEWaite/bin.git
        at java.base/java.net.URL.<init>(URL.java:644)
        at java.base/java.net.URL.<init>(URL.java:540)
        at java.base/java.net.URL.<init>(URL.java:487)
        at org.jenkinsci.plugins.github_branch_source.GitHubRepositoryInfo.forRepositoryUrl(GitHubRepositoryInfo.java:87)
Caused: java.lang.IllegalArgumentException
        at org.jenkinsci.plugins.github_branch_source.GitHubRepositoryInfo.forRepositoryUrl(GitHubRepositoryInfo.java:89)
        at org.jenkinsci.plugins.github_branch_source.GitHubRepoSizeEstimator$RepositorySizeGithubAPI.isApplicableTo(GitHubRepoSizeEstimator.java:23)
        at jenkins.plugins.git.GitToolChooser.lambda$setSizeFromAPI$0(GitToolChooser.java:120)
        at java.base/java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:176)
        at java.base/java.util.Iterator.forEachRemaining(Iterator.java:133)
        at java.base/java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:484)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:474)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:913)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:578)
        at jenkins.plugins.git.GitToolChooser.setSizeFromAPI(GitToolChooser.java:121)
        at jenkins.plugins.git.GitToolChooser.decideAndUseAPI(GitToolChooser.java:106)
        at jenkins.plugins.git.GitToolChooser.<init>(GitToolChooser.java:73)
        at hudson.plugins.git.GitSCM.createClient(GitSCM.java:852)
        at hudson.plugins.git.GitSCM.compareRemoteRevisionWithImpl(GitSCM.java:710)
        at hudson.plugins.git.GitSCM.compareRemoteRevisionWith(GitSCM.java:677)
        at hudson.scm.SCM.compareRemoteRevisionWith(SCM.java:401)
        at hudson.scm.SCM.poll(SCM.java:418)
        at hudson.model.AbstractProject._poll(AbstractProject.java:1388)
        at hudson.model.AbstractProject.poll(AbstractProject.java:1291)
        at hudson.triggers.SCMTrigger$Runner.runPolling(SCMTrigger.java:606)
        at hudson.triggers.SCMTrigger$Runner.run(SCMTrigger.java:652)
        at hudson.util.SequentialExecutionQueue$QueueEntry.run(SequentialExecutionQueue.java:119)
        at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
        at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
        at java.base/java.lang.Thread.run(Thread.java:834)

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 https://github.com/jenkinsci/git-plugin/pull/947 has shown me that can be complicated when we're trying to work with multiple SCM providers (like Bitbucket).

MarkEWaite avatar Sep 04 '20 17:09 MarkEWaite

An instance of the illegal argument exception blocks my attempt to clone an Assembla repository as well. The stack trace in the job is:

FATAL: Invalid repository URL: https://git.assembla.com/git-plugin.bin.git
java.lang.IllegalArgumentException: Invalid repository URL: https://git.assembla.com/git-plugin.bin.git
	at org.jenkinsci.plugins.github_branch_source.GitHubRepositoryInfo.forRepositoryUrl(GitHubRepositoryInfo.java:97)
	at org.jenkinsci.plugins.github_branch_source.GitHubRepoSizeEstimator$RepositorySizeGithubAPI.isApplicableTo(GitHubRepoSizeEstimator.java:23)
	at jenkins.plugins.git.GitToolChooser.lambda$setSizeFromAPI$0(GitToolChooser.java:120)
	at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:174)
	at java.util.Iterator.forEachRemaining(Iterator.java:116)
	at java.util.Spliterators$IteratorSpliterator.forEachRemaining(Spliterators.java:1801)
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:482)
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:472)
	at java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:708)
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234)
	at java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:566)
	at jenkins.plugins.git.GitToolChooser.setSizeFromAPI(GitToolChooser.java:121)
	at jenkins.plugins.git.GitToolChooser.decideAndUseAPI(GitToolChooser.java:106)
	at jenkins.plugins.git.GitToolChooser.<init>(GitToolChooser.java:73)
	at hudson.plugins.git.GitSCM.createClient(GitSCM.java:852)
	at hudson.plugins.git.GitSCM.createClient(GitSCM.java:833)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1251)
	at hudson.scm.SCM.checkout(SCM.java:505)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1206)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
	at hudson.model.Run.execute(Run.java:1894)
	at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:428)

MarkEWaite avatar Sep 05 '20 09:09 MarkEWaite

@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 GitHubRepositoryInfo info = GitHubRepositoryInfo.forRepositoryUrl(repoUrl); raises an exception for URLs belonging to other providers or different protocol than the HTTPS.

I've added those changes in the latest commits, now I hope that you won't see these issues while running this patch.

rishabhBudhouliya avatar Sep 05 '20 10:09 rishabhBudhouliya

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).

@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.

rishabhBudhouliya avatar Sep 05 '20 11:09 rishabhBudhouliya