• Blue Ocean 1.4 - beta 2

      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

          [JENKINS-43194] Lightweight checkout for PR merge jobs

          Jesse Glick added a comment -

          In the case of the merge_commit_sha commit, it should have two parents that we can check and they should match up to the head and base commits.

          It should have two parents, and they should be some PR commit and some base branch commit. Not necessarily the ones encoded in PullRequestSCMRevision.

          Jesse Glick added a comment - In the case of the merge_commit_sha commit, it should have two parents that we can check and they should match up to the head and base commits. It should have two parents, and they should be some PR commit and some base branch commit. Not necessarily the ones encoded in PullRequestSCMRevision .

          Liam Newman added a comment -

          jglick
          Just to keep this on track my original question was:
          From what I can see merge_commit_sha should be what we're using for "Merge Strategy" pipeline jobs, what am I missing?

          Points covered so far:
          1. (resolved) Deprecated - they later decided not to deprecate it.
          2. (workaround possible) Not generated consistently - May require a little hacking to trigger update, but that is doable.
          3. (open) Not verifiably accurate - The parents of merge_commit_sha should be the PR head and base. You said maybe not. I don't understand your point there. In what case do we want to allow the parents of the merge_commit_sha not to match to the ones on the PullRequestSCMRevision ?

          As I said, you have way more knowledge in this area and I really appreciate your help in drilling into this.

          Liam Newman added a comment - jglick Just to keep this on track my original question was: From what I can see merge_commit_sha should be what we're using for "Merge Strategy" pipeline jobs, what am I missing? Points covered so far: 1. (resolved) Deprecated - they later decided not to deprecate it. 2. (workaround possible) Not generated consistently - May require a little hacking to trigger update, but that is doable. 3. (open) Not verifiably accurate - The parents of merge_commit_sha should be the PR head and base. You said maybe not. I don't understand your point there. In what case do we want to allow the parents of the merge_commit_sha not to match to the ones on the PullRequestSCMRevision ? As I said, you have way more knowledge in this area and I really appreciate your help in drilling into this.

          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.

          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.

          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.

          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.

          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.

          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.

          Liam Newman added a comment - - edited

          jglick

          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.

          Liam Newman added a comment - - edited jglick 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.

          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

          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

          Liam Newman added a comment -

          Released in v2.5.0.

          Liam Newman added a comment - Released in v2.5.0.

          James F. added a comment -

          I have been experiencing problems since the most recent update of this plugin to 2.5.0.  I'm running the latest Jenkins with all plugins updated.  After updating from 2.4.5 of this plugin to 2.5.0 my tests are failing with the following error.  As soon as I roll the plugin back to 2.4.5 the tests being passing again.  Anyone have any idea what might be causing this?

           

          Caused: org.kohsuke.github.GHFileNotFoundException:

          {"message":"No commit found for SHA: xxxxxxxxxxxxxxdc556f036","documentation_url":"[https://developer.github.com/v3/repos/commits/#get-a-single-commit]"}

          James F. added a comment - I have been experiencing problems since the most recent update of this plugin to 2.5.0.  I'm running the latest Jenkins with all plugins updated.  After updating from 2.4.5 of this plugin to 2.5.0 my tests are failing with the following error.  As soon as I roll the plugin back to 2.4.5 the tests being passing again.  Anyone have any idea what might be causing this?   Caused: org.kohsuke.github.GHFileNotFoundException: {"message":"No commit found for SHA: xxxxxxxxxxxxxxdc556f036","documentation_url":"[https://developer.github.com/v3/repos/commits/#get-a-single-commit]"}

          Liam Newman added a comment -

          silverorange
          Please open a new issue with more details including callstacks.
          As a work around please try closing and reopening the PR that is causing the failure.

          Liam Newman added a comment - silverorange Please open a new issue with more details including callstacks. As a work around please try closing and reopening the PR that is causing the failure.

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

              Created:
              Updated:
              Resolved: