Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-43194

Lightweight checkout for PR merge jobs

    XMLWordPrintable

    Details

    • Sprint:
      Blue Ocean 1.4 - beta 2
    • Similar Issues:

      Description

      Please pull in the changes made to the SCM API to reduce the number of checkouts required for pipelines. Related to JENKINS-33273

       

      There was a mention in the referenced issue about PRs needing special treatment: "there is no implementation currently for github-branch-source-plugin in the case of a PR job configured to merge with the base branch, as GitHub does not offer an API dedicated to this purpose. In the case that the merge can be assumed to be a fast-forward (there is no base branch change subsequent to the common ancestor), this plugin could in principle load content via API from the PR branch; it would still need to fall back to full checkout and Git merge otherwise." - https://issues.jenkins-ci.org/browse/JENKINS-33273?focusedCommentId=292273#comment-292273

        Attachments

          Issue Links

            Activity

            Hide
            jglick Jesse Glick added a comment -

            We have no control over what the parents of merge_commit_sha are. They may well include the PR’s current head and some commit from the base branch. Those are not, in general, going to be identical to what is specified in PullRequestSCMRevision for a given build.

            Show
            jglick Jesse Glick added a comment - We have no control over what the parents of merge_commit_sha are. They may well include the PR’s current head and some commit from the base branch. Those are not, in general, going to be identical to what is specified in PullRequestSCMRevision for a given build.
            Hide
            stevenfoster Steven Foster added a comment -

            Is it feasible to apply this as an optimization, even if it only applies a certain percentage of time, with the current behavior as a fallback? I guess it depends on if the parents of merge_commit_sha are cheaper to inspect than cloning the repo.

            Show
            stevenfoster Steven Foster added a comment - Is it feasible to apply this as an optimization, even if it only applies a certain percentage of time, with the current behavior as a fallback? I guess it depends on if the parents of merge_commit_sha are cheaper to inspect than cloning the repo.
            Hide
            jglick Jesse Glick added a comment -

            We cannot use merge_commit_sha from this plugin. Just forget it exists.

            W.r.t. optimizations: it is possible for GitHubSCMFileSystem.BuilderImpl.build to simply fall back to heavyweight checkouts if its preconditions are not met. In fact this is what it already does—the issue is that it does so for all cases of PullRequestSCMHead.merge, but this is a common case that we want lightweight checkout for. My suggestion from 2017-06-19 was to fail with a meaningful message rather than falling back to heavyweight checkout in case the merge is not fast-forward for the fie in question, since we do not really want the big clone on the master ever and the fix for the user is simple (just bring your PR up to date).

            Anyway it seems that quietly falling back to heavyweight checkout would not work without bigger changes. SCMBinder checks for the existence of an SCMFileSystem at this revision and then uses SCMFileSystem.child(String) to load the Jenkinsfile or whatever. This is because scm-api does not allow the client to communicate to the implementation that it only cares about a single file: the decision to return null or not happens before the filename is used. So what we can do is to unconditionally return the SCMFileSystem but then make GitHubSCMFile.content throw the AbortException if that particular file cannot in fact be loaded via straightforward GitHub API calls (i.e., if it has diverged between the two parent commits from their merge base). Now it happens that SCMBinder currently falls back to heavyweight checkout on an IOException as well as a null return value from SCMFileSystem.of, which is something we could consider changing, or making customizable.

            Show
            jglick Jesse Glick added a comment - We cannot use merge_commit_sha from this plugin. Just forget it exists. W.r.t. optimizations: it is possible for GitHubSCMFileSystem.BuilderImpl.build to simply fall back to heavyweight checkouts if its preconditions are not met. In fact this is what it already does—the issue is that it does so for all cases of PullRequestSCMHead.merge , but this is a common case that we want lightweight checkout for. My suggestion from 2017-06-19 was to fail with a meaningful message rather than falling back to heavyweight checkout in case the merge is not fast-forward for the fie in question, since we do not really want the big clone on the master ever and the fix for the user is simple (just bring your PR up to date). Anyway it seems that quietly falling back to heavyweight checkout would not work without bigger changes. SCMBinder checks for the existence of an SCMFileSystem at this revision and then uses SCMFileSystem.child(String) to load the Jenkinsfile or whatever. This is because scm-api does not allow the client to communicate to the implementation that it only cares about a single file: the decision to return null or not happens before the filename is used. So what we can do is to unconditionally return the SCMFileSystem but then make GitHubSCMFile.content throw the AbortException if that particular file cannot in fact be loaded via straightforward GitHub API calls (i.e., if it has diverged between the two parent commits from their merge base). Now it happens that SCMBinder currently falls back to heavyweight checkout on an IOException as well as a null return value from SCMFileSystem.of , which is something we could consider changing, or making customizable.
            Hide
            bitwiseman Liam Newman added a comment - - edited

            Jesse Glick

            We cannot use merge_commit_sha from this plugin. Just forget it exists.

            Why? I need to understand why this is not possible. I don't need you to spoon feed it to me but I do need more information than you've provided.

            The suggestion that we should fail and make the user bring their PR up-to-date is an interesting one. I'll add that to the list of options.

            Show
            bitwiseman Liam Newman added a comment - - edited Jesse Glick We cannot use merge_commit_sha from this plugin. Just forget it exists. Why? I need to understand why this is not possible. I don't need you to spoon feed it to me but I do need more information than you've provided. The suggestion that we should fail and make the user bring their PR up-to-date is an interesting one. I'll add that to the list of options.
            Hide
            bitwiseman Liam Newman added a comment -

            I've released a beta release of this change in v2.4.6-beta-1:

            This is not production ready but anyone wanting to test it and give feedback would be appreciated. It is on the experimental update site.
            Also: https://repo.jenkins-ci.org/releases/org/jenkins-ci/plugins/github-branch-source/2.4.6-beta-1/github-branch-source-2.4.6-beta-1.hpi

            Show
            bitwiseman Liam Newman added a comment - I've released a beta release of this change in v2.4.6-beta-1: This is not production ready but anyone wanting to test it and give feedback would be appreciated. It is on the experimental update site. Also: https://repo.jenkins-ci.org/releases/org/jenkins-ci/plugins/github-branch-source/2.4.6-beta-1/github-branch-source-2.4.6-beta-1.hpi

              People

              • Assignee:
                bitwiseman Liam Newman
                Reporter:
                maxpowa Max Gurela
              • Votes:
                11 Vote for this issue
                Watchers:
                21 Start watching this issue

                Dates

                • Created:
                  Updated: