JENKINS-48737 Adding lightweight checkouts for Bitbucket Pull Requests #117

benjaminfuchs · 2018-03-23T19:04:26Z

Proposing a fix for https://issues.jenkins-ci.org/browse/JENKINS-48737

Tested on our Bitbucket -> Jenkins setup.

tarioch · 2018-03-24T22:42:55Z

#114 seems to make more sense than this fix as it fixes the source issue

benjaminfuchs · 2018-03-25T00:54:08Z

@tarioch I totally agree, but this fix will just affect the light weight checkout. So compared to #114 it just fixes this problem and doesn't change the environment variable CHANGE_BRANCH. But if #114 get merged this is not needed anymore.

BrunoLavitForgerock · 2018-04-04T08:29:59Z

FYI, we have applied #117 and #111 on the master branch and the lightweight checkout is now working for our postcommit and PR pipeline jobs.
We are using Bitbucket datacenter 5.8.0 and Jenkins 2.46.3.

jetersen · 2018-04-04T08:43:32Z

...ain/java/com/cloudbees/jenkins/plugins/bitbucket/server/client/BitbucketServerAPIClient.java

+        String ref;
+        if(file.getRef().matches("PR-\\d+")) {  // because JENKINS-48737
+            String prId = file.getRef().replace("PR-", "");
+            ref = "refs/pull-requests/" + prId + "/from";


Could respect the pull request merge strategy set on the build configuration

agree with @Casz, could we use pull-requests/id/merge as ref for merge strategy?

indeed, which I was hinting at 👍

Thanks for the feedback. I will pick up your comments. Hopefully I have time this week. 👍

honnel · 2018-04-24T08:45:06Z

We are affected by this issue, too. Any plans for next release?

benjaminfuchs · 2018-06-09T14:16:08Z

@Casz @fengxx Finally I had time reworking this. I changed it, so the checkout strategy is taken into account.

benjaminfuchs · 2018-06-11T08:02:18Z

So I tested it in our environment. It is working but this print is now wrong "Obtained Jenkinsfile from XXXX" since it should mention, that it used the merge ref.
Also if you choose "Merging the pull request with the current target branch revision" it is still using heavy weight checkout and doesn't even try leight weight version. Seems there have to be more changes to support both checkout strategies.

jetersen · 2018-06-11T08:18:59Z

@benjaminfuchs see #114

benjaminfuchs · 2018-06-11T16:39:47Z

@Casz If 114 also fixes this issue and gets merged I'm also fine with that. Is this done with the next release?
In the meantime this fix would be good as a workaround before 114 is merged and if it's merged it won't get triggers anymore.
As 114 is not solving the problem with the "merge" checkout strategy I would continue working on this in this PR?

bjornmagnusson · 2018-07-01T21:43:05Z

What is missing for getting this merged? This is blocker for us.

amuniz · 2018-07-12T13:59:55Z

@tarioch said this issue is also fixed by #114, did someone check that?

tarioch · 2018-07-13T06:07:16Z

As far as I can tell #114 fixes this issue for the "current pull request revision" strategy but it doesn't fix it for the "merge" strategy. Correct me please @benjaminfuchs

benjaminfuchs · 2018-07-15T08:41:14Z

@tarioch Yes #114 will fix the "current pull request revision" but not the "merge" strategy. I think the "merge" strategy is the most common use-case, so I guess this pull request should be merged independent from #114.
We are using this in our productive environment and would be happy if this could be merged. Tests are implemented. If there is a reason for not merging let me know @Casz @fengxx

tarioch · 2018-07-16T05:57:08Z

@benjaminfuchs I think after #114 is merged the first part

if (ref.matches("PR-\\d+")) { // because JENKINS-48737

would need to be changed because I think that would then be the branch name of the PR.

bjornmagnusson · 2018-08-21T15:31:46Z

Any updates?

jetersen · 2018-09-22T05:01:36Z

I'll try and look into this and #114

benjaminfuchs · 2018-09-25T15:05:26Z

@Casz Did you plan to finish this? After #114 is merged I would try to have a look in this one this weekend. Just want sync to avoid double work.

jetersen · 2018-10-07T14:09:03Z

@benjaminfuchs please look into it 👍

bjornmagnusson · 2018-10-26T10:38:39Z

any ETA on this?

jetersen · 2018-10-27T15:46:56Z

@benjaminfuchs I checked and PR still does full checkouts.

This needs to adapt:

bitbucket-branch-source-plugin/src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFileSystem.java

Lines 137 to 141 in f442c48

    
           PullRequestSCMHead pr = (PullRequestSCMHead) head; 
        
           if (!(pr.getCheckoutStrategy() == ChangeRequestCheckoutStrategy.MERGE) && pr.getRepository() != null) { 
        
               return new BitbucketSCMFileSystem(apiClient, pr.getOriginName(), rev); 
        
           } 
        
           return null; // TODO support merge revisions somehow

jetersen · 2018-10-27T16:00:32Z

Actually ~~most~~ all of the logic can be moved to that spot.

jetersen · 2018-10-27T16:12:42Z

@Override
public SCMFileSystem build(@NonNull SCMSource source, @NonNull SCMHead head, @CheckForNull SCMRevision rev)
        throws IOException, InterruptedException {
    BitbucketSCMSource src = (BitbucketSCMSource) source;
    
    String credentialsId = src.getCredentialsId();
    String owner = src.getRepoOwner();
    String repository = src.getRepository();
    String serverUrl = src.getServerUrl();
    StandardUsernamePasswordCredentials credentials;
    credentials = lookupScanCredentials(src.getOwner(), credentialsId);
    
    BitbucketApi apiClient = BitbucketApiFactory.newInstance(serverUrl, credentials, owner, repository);
    String ref;
    if (head instanceof BranchSCMHead) {
        ref = head.getName();
    } else if (head instanceof PullRequestSCMHead) {
        PullRequestSCMHead pr = (PullRequestSCMHead) head;
        String checkoutStrat = "from";
        if (pr.getCheckoutStrategy() == ChangeRequestCheckoutStrategy.MERGE) {
            checkoutStrat = "merge";
        }
        ref = "refs/pull-requests/" + pr.getId() + "/" + checkoutStrat;
    } else if (head instanceof BitbucketTagSCMHead) {
         ref = "tags/" + head.getName();
    } else {
        return null;
    }

    return new BitbucketSCMFileSystem(apiClient, ref, rev);
}

works out nicely 😅

benjaminfuchs · 2018-10-27T16:25:14Z

@Casz We are currently using my patched plugin and it works fine. Your change also looks reasonable. How should we continue?

jetersen · 2018-10-27T16:33:32Z

sadly does not work for cloud

java.io.FileNotFoundException: URL: https://api.bitbucket.org/2.0/repositories/caszum/testrepo/src/a84e84a71c6cc81c23bdda3fc216867a7e6fe031%2B4a3423ac3e042cfeb3b1a09e57280e7d8385ac86/Jenkinsfile
	at com.cloudbees.jenkins.plugins.bitbucket.client.BitbucketCloudApiClient.getRequestAsInputStream(BitbucketCloudApiClient.java:745)
	at com.cloudbees.jenkins.plugins.bitbucket.client.BitbucketCloudApiClient.getFileContent(BitbucketCloudApiClient.java:927)
	at com.cloudbees.jenkins.plugins.bitbucket.filesystem.BitbucketSCMFile.content(BitbucketSCMFile.java:109)
	at jenkins.scm.api.SCMFile.contentAsString(SCMFile.java:338)
	at org.jenkinsci.plugins.workflow.multibranch.SCMBinder.create(SCMBinder.java:104)
	at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:303)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:421)

benjaminfuchs · 2018-10-27T16:39:00Z

@Casz I guess my changes only work if you set "Discover pull requests from origin" to Strategy "The current pull requets revision". EDIT: Its not a guess, just downloaded the latest jenkins build result here and can confirm. Only works with that setting. Why, currently dont know.

jetersen · 2018-10-27T16:42:34Z

I guess we have to check api client if server or cloud for now... until we can find a solution for cloud too

jetersen · 2018-10-27T16:43:38Z

with the changes from #114 this did not work at least the current state 😅

jetersen · 2018-10-27T17:18:53Z

@benjaminfuchs if you could adapt the PR and the tests to:

if (head instanceof BranchSCMHead) {
    ref = head.getName();
} else if (head instanceof PullRequestSCMHead) {
    PullRequestSCMHead pr = (PullRequestSCMHead) head;
    if (apiClient instanceof BitbucketCloudApiClient) {
        if (!(pr.getCheckoutStrategy() == ChangeRequestCheckoutStrategy.MERGE) && pr.getRepository() != null) {
            return new BitbucketSCMFileSystem(apiClient, pr.getOriginName(), rev);
        }
        return null; // TODO support merge revisions somehow for cloud
    }
    String checkoutStrategy = "from";
    if (pr.getCheckoutStrategy() == ChangeRequestCheckoutStrategy.MERGE) {
        checkoutStrategy = "merge";
    }
    ref = "refs/pull-requests/" + pr.getId() + "/" + checkoutStrategy;
} else if (head instanceof BitbucketTagSCMHead) {
     ref = "tags/" + head.getName();
} else {
    return null;
}

I would appreciate it 😇🙌

darxriggs · 2018-11-20T22:41:04Z

src/main/java/com/cloudbees/jenkins/plugins/bitbucket/filesystem/BitbucketSCMFile.java

+	@CheckForNull
+	private ChangeRequestCheckoutStrategy getCheckoutStrategy() throws IOException, InterruptedException {
+		if (this.isDirectory()) {
+			SCMRevision revision = fileSystem.getRevision();


Having a look at the constructors in line 76 and line 80 fileSystem could be null.
I would also mark this variable final and maybe @CheckForNull.

bjornmagnusson · 2018-12-13T20:42:04Z

Any ETA for this very important feature?

jetersen · 2018-12-13T23:18:20Z

@bjornmagnusson if you feel it is important, feel free to rework the PR.
I'll leave this here.
https://snarky.ca/setting-expectations-for-open-source-participation/

stale · 2019-04-06T05:43:46Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

jetersen · 2019-04-07T03:06:29Z

Superseded by #174

JENKINS-48737 Adding lightweight checkouts for Bitbucket Pull Requests

3bac95e

This was referenced Apr 4, 2018

JENKINS-49671 Check if file exists - available for file in subdirectory #111

Merged

fix JENKINS-47617 CHANGE_BRANCH should return source branch name #114

Merged

jetersen reviewed Apr 4, 2018

View reviewed changes

benjaminfuchs force-pushed the master branch 7 times, most recently from 26b9f6a to 3bac95e Compare April 8, 2018 22:29

benjaminfuchs force-pushed the master branch 6 times, most recently from fcb4ed0 to 1e7dbf8 Compare June 9, 2018 14:05

JENKINS-48737 Taking checkout strategy into account.

faf745e

benjaminfuchs force-pushed the master branch from 1e7dbf8 to faf745e Compare June 9, 2018 14:11

benjaminfuchs mentioned this pull request Jun 13, 2018

Checkout only Jenkinsfile even for PR #123

Closed

michelzanini mentioned this pull request Aug 2, 2018

Avoid notifying Bitbucket multiple times #130

Merged

darxriggs reviewed Nov 20, 2018

View reviewed changes

jetersen mentioned this pull request Mar 1, 2019

JENKINS-48737 Fixing lightweight pull request checkouts, also for files in subdirectories #174

Merged

stale bot added the stale label Apr 6, 2019

jetersen closed this Apr 7, 2019

JENKINS-48737 Adding lightweight checkouts for Bitbucket Pull Requests #117

JENKINS-48737 Adding lightweight checkouts for Bitbucket Pull Requests #117

Conversation

benjaminfuchs commented Mar 23, 2018

tarioch commented Mar 24, 2018

benjaminfuchs commented Mar 25, 2018 • edited Loading

BrunoLavitForgerock commented Apr 4, 2018 • edited Loading

jetersen Apr 4, 2018

Choose a reason for hiding this comment

fengxx Apr 9, 2018

Choose a reason for hiding this comment

jetersen Apr 9, 2018

Choose a reason for hiding this comment

benjaminfuchs Apr 9, 2018

Choose a reason for hiding this comment

honnel commented Apr 24, 2018

benjaminfuchs commented Jun 9, 2018

benjaminfuchs commented Jun 11, 2018

jetersen commented Jun 11, 2018

benjaminfuchs commented Jun 11, 2018 • edited Loading

bjornmagnusson commented Jul 1, 2018

amuniz commented Jul 12, 2018

tarioch commented Jul 13, 2018

benjaminfuchs commented Jul 15, 2018 • edited Loading

tarioch commented Jul 16, 2018

bjornmagnusson commented Aug 21, 2018

jetersen commented Sep 22, 2018

benjaminfuchs commented Sep 25, 2018

jetersen commented Oct 7, 2018

bjornmagnusson commented Oct 26, 2018

jetersen commented Oct 27, 2018

jetersen commented Oct 27, 2018 • edited Loading

jetersen commented Oct 27, 2018 • edited Loading

benjaminfuchs commented Oct 27, 2018

jetersen commented Oct 27, 2018 • edited Loading

benjaminfuchs commented Oct 27, 2018 • edited Loading

jetersen commented Oct 27, 2018

jetersen commented Oct 27, 2018 • edited Loading

jetersen commented Oct 27, 2018 • edited Loading

darxriggs Nov 20, 2018

Choose a reason for hiding this comment

bjornmagnusson commented Dec 13, 2018

jetersen commented Dec 13, 2018

stale bot commented Apr 6, 2019

jetersen commented Apr 7, 2019

benjaminfuchs commented Mar 25, 2018 •

edited

Loading

BrunoLavitForgerock commented Apr 4, 2018 •

edited

Loading

benjaminfuchs commented Jun 11, 2018 •

edited

Loading

benjaminfuchs commented Jul 15, 2018 •

edited

Loading

jetersen commented Oct 27, 2018 •

edited

Loading

jetersen commented Oct 27, 2018 •

edited

Loading

jetersen commented Oct 27, 2018 •

edited

Loading

benjaminfuchs commented Oct 27, 2018 •

edited

Loading

jetersen commented Oct 27, 2018 •

edited

Loading

jetersen commented Oct 27, 2018 •

edited

Loading