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

"Merge if necessary" submit type builds the wrong commit for change-merged

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Open (View Workflow)
    • Priority: Critical
    • Resolution: Unresolved
    • Component/s: gerrit-trigger-plugin
    • Labels:
      None
    • Environment:
      Jenkins 2.277.2
      gerrit-trigger 2.33.0
      Gerrit 3.2.3
    • Similar Issues:

      Description

      When using the "Merge if necessary" submit type with Gerrit and the change-merged build trigger, if Gerrit did an automatic merge commit, the commit-ish to build will be the sha1 of the commit from the patchset prior to the merge commit resulting in the incorrect checkout of the repository content and causing an improper artifact to be built and published. Further if the merge commit merged changes that were made to the Jenkinsfile, the build will not even be using the correct pipeline code. Since either case can result in incorrect but possibly successful build results, I set the priority of this bug to Critical.

      If this is a bug, change-merged events should be fetching based on the refName and checking out the newRev.

      If this is not a bug in the behavior, then I believe this should be addressed in documentation as it is a common for Gerrit repos to be configured with "Merge if necessary" and following the current documentation will result in incorrect build outputs. To work around this, I have to configure multiple Jenkins jobs, one for patchset-created and one for ref-updated because I cannot figure out any way to trigger on both of those events from the same job and have the correct content checked out in this merge scenario. Also, when I build on ref-updated, there is no feedback comment to the changeset that a build was started and what its final result was.

      I experimented with this a bit using a single job configured with patchset-created and change-merged triggers and pipeline scm checkout configured the way the gerrit-trigger documentation suggests here: https://plugins.jenkins.io/gerrit-trigger/#usage-with-the-git-plugin

      When you let the gerrit-trigger plugin choose what to build, you get:

      • For a ref-updated event, commit-ish to build is based on GERRIT_NEWREV and branch name is based on GERRIT_REFNAME
      • For a patchset-created or change-merged event, commit-ish to build is based on patchset revision which is the commit-ish associated with the change GERRIT_PATCHSET_REVISION and branch name is the ref GERRIT_REFSPEC
      • For a manually triggered build, the values come from the Jenkins pipeline configuration properties. The commit-ish to build will fall back to "FETCH_HEAD" which will depend on the Refspec setting of the Advanced part of that configuration and the branch will be set based on the Branch setting.

      This is based on analysis of the Jenkins plugin code for the version of the plugin we currently have deployed, not documentation, see: https://github.com/jenkinsci/gerrit-trigger-plugin/blob/gerrit-trigger-2.33.0/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerBuildChooser.java#L106

      I'm fairly cetain that code is in play but not 100% as I don't follow the whole code base and Jenkins interactions.

      I captured the Gerrit events during a merge from the Gerrit events-log REST API:

      curl -uuser:pass "http://mygerritserver/a/plugins/events-log/events/?t1=2021-04-26"

      These are the two that fired the builds for the patchset-created events:

      {"uploader":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"patchSet":{"number":1,"revision":"788bfd7ba510dcceef0ca7b6bf830e1300c358f8","parents":["bffc495c8e33e14cf642aaa535f1779bf0652646"],"ref":"refs/changes/43/1443/1","uploader":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"createdOn":1619462659,"author":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"kind":"REWORK","sizeInsertions":1,"sizeDeletions":1},"change":{"project":"cdptestgroup/eric-test-empty-pipeline-01","branch":"master","id":"I3de8c8757859bf730703c9a12966354663caf2f8","number":1443,"subject":"ci: setup for an automatic merge commit take 2 (CDPINT-8097)","owner":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"url":"http://gerrit-cdptest.unx.sas.com/c/cdptestgroup/eric-test-empty-pipeline-01/+/1443","commitMessage":"ci: setup for an automatic merge commit take 2 (CDPINT-8097)\n\nChange-Id: I3de8c8757859bf730703c9a12966354663caf2f8\n","createdOn":1619462659,"status":"NEW"},"project":{"name":"cdptestgroup/eric-test-empty-pipeline-01"},"refName":"refs/heads/master","changeKey":{"key":"I3de8c8757859bf730703c9a12966354663caf2f8"},"type":"patchset-created","eventCreatedOn":1619462660}
       {"uploader":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"patchSet":{"number":1,"revision":"7552e71c255bad9fab899e6350e542ea34405250","parents":["bffc495c8e33e14cf642aaa535f1779bf0652646"],"ref":"refs/changes/44/1444/1","uploader":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"createdOn":1619462747,"author":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"kind":"REWORK","sizeInsertions":1,"sizeDeletions":0},"change":{"project":"cdptestgroup/eric-test-empty-pipeline-01","branch":"master","id":"Ie998d23c8b14b964ea66071b79f9be97c85c7656","number":1444,"subject":"ci: second commit for an automatic merge commit take 2 (CDPINT-8097)","owner":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"url":"http://gerrit-cdptest.unx.sas.com/c/cdptestgroup/eric-test-empty-pipeline-01/+/1444","commitMessage":"ci: second commit for an automatic merge commit take 2 (CDPINT-8097)\n\nChange-Id: Ie998d23c8b14b964ea66071b79f9be97c85c7656\n","createdOn":1619462747,"status":"NEW"},"project":{"name":"cdptestgroup/eric-test-empty-pipeline-01"},"refName":"refs/heads/master","changeKey":{"key":"Ie998d23c8b14b964ea66071b79f9be97c85c7656"},"type":"patchset-created","eventCreatedOn":1619462747}

      These are the two that fired the builds for the change-merged events (note the newRev in the second event which points at our merge commit which I expected to be built but the choosing strategy is using the patchSet.revision):

      {"submitter":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"newRev":"788bfd7ba510dcceef0ca7b6bf830e1300c358f8","patchSet":{"number":1,"revision":"788bfd7ba510dcceef0ca7b6bf830e1300c358f8","parents":["bffc495c8e33e14cf642aaa535f1779bf0652646"],"ref":"refs/changes/43/1443/1","uploader":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"createdOn":1619462659,"author":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"kind":"REWORK","sizeInsertions":1,"sizeDeletions":1},"change":{"project":"cdptestgroup/eric-test-empty-pipeline-01","branch":"master","id":"I3de8c8757859bf730703c9a12966354663caf2f8","number":1443,"subject":"ci: setup for an automatic merge commit take 2 (CDPINT-8097)","owner":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"url":"http://gerrit-cdptest.unx.sas.com/c/cdptestgroup/eric-test-empty-pipeline-01/+/1443","commitMessage":"ci: setup for an automatic merge commit take 2 (CDPINT-8097)\n\nChange-Id: I3de8c8757859bf730703c9a12966354663caf2f8\n","createdOn":1619462659,"status":"MERGED"},"project":{"name":"cdptestgroup/eric-test-empty-pipeline-01"},"refName":"refs/heads/master","changeKey":{"key":"I3de8c8757859bf730703c9a12966354663caf2f8"},"type":"change-merged","eventCreatedOn":1619462800}
       {"submitter":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"newRev":"ae9ad9f4e31c34e1d245997471d666b4430b4178","patchSet":{"number":1,"revision":"7552e71c255bad9fab899e6350e542ea34405250","parents":["bffc495c8e33e14cf642aaa535f1779bf0652646"],"ref":"refs/changes/44/1444/1","uploader":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"createdOn":1619462747,"author":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"kind":"REWORK","sizeInsertions":1,"sizeDeletions":0},"change":{"project":"cdptestgroup/eric-test-empty-pipeline-01","branch":"master","id":"Ie998d23c8b14b964ea66071b79f9be97c85c7656","number":1444,"subject":"ci: second commit for an automatic merge commit take 2 (CDPINT-8097)","owner":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"url":"http://gerrit-cdptest.unx.sas.com/c/cdptestgroup/eric-test-empty-pipeline-01/+/1444","commitMessage":"ci: second commit for an automatic merge commit take 2 (CDPINT-8097)\n\nChange-Id: Ie998d23c8b14b964ea66071b79f9be97c85c7656\n","createdOn":1619462747,"status":"MERGED"},"project":{"name":"cdptestgroup/eric-test-empty-pipeline-01"},"refName":"refs/heads/master","changeKey":{"key":"Ie998d23c8b14b964ea66071b79f9be97c85c7656"},"type":"change-merged","eventCreatedOn":1619462857}

      Here are the related ref-updated events:

      {"submitter":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"refUpdate":{"oldRev":"bffc495c8e33e14cf642aaa535f1779bf0652646","newRev":"788bfd7ba510dcceef0ca7b6bf830e1300c358f8","refName":"refs/heads/master","project":"cdptestgroup/eric-test-empty-pipeline-01"},"type":"ref-updated","eventCreatedOn":1619462800}
       {"submitter":{"name":"My User","email":"My.User@mycompany.com","username":"myuser"},"refUpdate":{"oldRev":"788bfd7ba510dcceef0ca7b6bf830e1300c358f8","newRev":"ae9ad9f4e31c34e1d245997471d666b4430b4178","refName":"refs/heads/master","project":"cdptestgroup/eric-test-empty-pipeline-01"},"type":"ref-updated","eventCreatedOn":1619462857}

      Here is the git log after the merge:

      $ git log --oneline --graph HEAD~2..
       * ae9ad9f (HEAD -> master, origin/master, origin/HEAD) Merge "ci: second commit for an automatic merge commit take 2 (CDPINT-8097)"
       |\
       | * 7552e71 (mergeme) ci: second commit for an automatic merge commit take 2 (CDPINT-8097)
       * 788bfd7 ci: setup for an automatic merge commit take 2 (CDPINT-8097)

      These pipelines are configured setting the option `skipDefaultCheckout` to `true` and use the `checkout scm` step in a later stage. You can see in the Jenkins console for each of the triggered jobs, what was actually checked out:

       

      For the setup commit 788bfd7:

      • checkout the pipeline
      Checking out git ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git into /reps/jenkins/home/jobs/cdptestgroup_eric-test-empty-pipeline-01/workspace@script to read ci/Jenkinsfile
       The recommended git tool is: NONE
       using credential mycreds
       > git rev-parse --resolve-git-dir /reps/jenkins/home/jobs/cdptestgroup_eric-test-empty-pipeline-01/workspace@script/.git # timeout=10
       Fetching changes from the remote Git repository
       > git config remote.origin.url ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git # timeout=10
       Fetching upstream changes from ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git
       > git --version # timeout=10
       > git --version # 'git version 2.31.1'
       using GIT_SSH to set credentials used to integrate with gerrit/gitlab
       > git fetch --tags --force --progress -- ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git refs/changes/43/1443/1 # timeout=10
       > git rev-parse 788bfd7ba510dcceef0ca7b6bf830e1300c358f8^{commit} # timeout=10
       Checking out Revision 788bfd7ba510dcceef0ca7b6bf830e1300c358f8 (refs/changes/43/1443/1)
       > git config core.sparsecheckout # timeout=10
       > git checkout -f 788bfd7ba510dcceef0ca7b6bf830e1300c358f8 # timeout=10
       Commit message: "ci: setup for an automatic merge commit take 2 (CDPINT-8097)"
       > git rev-parse FETCH_HEAD^{commit} # timeout=10
       > git rev-list --no-walk bffc495c8e33e14cf642aaa535f1779bf0652646 # timeout=10
      • then the checkout in the later stage
      [Pipeline] checkout
       14:46:51 The recommended git tool is: NONE
       14:46:51 using credential mycreds
       14:46:51 Fetching changes from the remote Git repository
       14:46:51 Checking out Revision 788bfd7ba510dcceef0ca7b6bf830e1300c358f8 (refs/changes/43/1443/1)
       14:46:51 Commit message: "ci: setup for an automatic merge commit take 2 (CDPINT-8097)"

      And for the second commit 7552e71:

      • checkout the pipeline (note the fetch here against the refspec doesn't contain our merge commit but the event does have refName of refs/heads/master which would)
      Checking out git ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git into /reps/jenkins/home/jobs/cdptestgroup_eric-test-empty-pipeline-01/workspace@script to read ci/Jenkinsfile
       The recommended git tool is: NONE
       using credential mycreds
       > git rev-parse --resolve-git-dir /reps/jenkins/home/jobs/cdptestgroup_eric-test-empty-pipeline-01/workspace@script/.git # timeout=10
       Fetching changes from the remote Git repository
       > git config remote.origin.url ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git # timeout=10
       Fetching upstream changes from ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git
       > git --version # timeout=10
       > git --version # 'git version 2.31.1'
       using GIT_SSH to set credentials used to integrate with gerrit/gitlab
       > git fetch --tags --force --progress -- ssh://myuser@mygerritserver:29418/cdptestgroup/eric-test-empty-pipeline-01.git refs/changes/44/1444/1 # timeout=10
       > git rev-parse 7552e71c255bad9fab899e6350e542ea34405250^{commit} # timeout=10
       Checking out Revision 7552e71c255bad9fab899e6350e542ea34405250 (refs/changes/44/1444/1)
       > git config core.sparsecheckout # timeout=10
       > git checkout -f 7552e71c255bad9fab899e6350e542ea34405250 # timeout=10
       Commit message: "ci: second commit for an automatic merge commit take 2 (CDPINT-8097)"
       > git rev-parse FETCH_HEAD^{commit} # timeout=10
       > git rev-list --no-walk bffc495c8e33e14cf642aaa535f1779bf0652646 # timeout=10

       

      • then the checkout in the later stage
        [Pipeline] checkout
         14:47:45 The recommended git tool is: NONE
         14:47:45 using credential mycreds
         14:47:45 Fetching changes from the remote Git repository
         14:47:46 Checking out Revision 7552e71c255bad9fab899e6350e542ea34405250 (refs/changes/44/1444/1)
         14:47:46 Commit message: "ci: second commit for an automatic merge commit take 2 (CDPINT-8097)"

        Attachments

          Activity

          Hide
          eisakson Eric Isakson added a comment -

          rsandell wondering if you have had a few minutes to digest this and whether you consider this a bug or behaves as designed and maybe should be addressed as documentation? If you consider it a bug, I can try to work on a fix but don't want to waste effort on that if you think this is not a bug. Thanks!

          Show
          eisakson Eric Isakson added a comment - rsandell  wondering if you have had a few minutes to digest this and whether you consider this a bug or behaves as designed and maybe should be addressed as documentation? If you consider it a bug, I can try to work on a fix but don't want to waste effort on that if you think this is not a bug. Thanks!
          Hide
          eisakson Eric Isakson added a comment -

          rsandell I've pushed a first cut at a fix for this here: https://github.com/eric-isakson/gerrit-trigger-plugin/tree/choose-newrev-for-change-merged 

          In trying this out on my Jenkins instance I ran into some problems due to the job configuration based on the instructions here: https://github.com/jenkinsci/gerrit-trigger-plugin/tree/master/docs#usage-with-the-git-plugin because that refspec is used to do the fetch on the repo and the fact that the automatic merge commit is not on the refs/changes/... ref, I had to modify it from:

          $GERRIT_REFSPEC

           
          to:
           

          $GERRIT_REFSPEC +refs/heads/$GERRIT_BRANCH:refs/remotes/origin/$GERRIT_BRANCH

           
          Without that change, the build will break when there is an automatic merge commit as the git checkout command will not be able to get the commit and the job will fail with:
           

          ERROR: Couldn't find any revision to build. Verify the repository and branch configuration for this job.
          ERROR: Maximum checkout retry attempts reached, aborting

           
          I'm guessing that will be a non-starter for this contribution. I'm also still wondering if you consider the current behavior to be a bug or not? The only alternative I can come up with is to add another choosing strategy that uses the new behavior so that people have a non-breaking migration path to the fix. Would you be interested in a contribution like that?

          Show
          eisakson Eric Isakson added a comment - rsandell  I've pushed a first cut at a fix for this here: https://github.com/eric-isakson/gerrit-trigger-plugin/tree/choose-newrev-for-change-merged   In trying this out on my Jenkins instance I ran into some problems due to the job configuration based on the instructions here: https://github.com/jenkinsci/gerrit-trigger-plugin/tree/master/docs#usage-with-the-git-plugin  because that refspec is used to do the fetch on the repo and the fact that the automatic merge commit is not on the refs/changes/... ref, I had to modify it from: $GERRIT_REFSPEC   to:   $GERRIT_REFSPEC +refs/heads/$GERRIT_BRANCH:refs/remotes/origin/$GERRIT_BRANCH   Without that change, the build will break when there is an automatic merge commit as the git checkout command will not be able to get the commit and the job will fail with:   ERROR: Couldn't find any revision to build. Verify the repository and branch configuration for this job. ERROR: Maximum checkout retry attempts reached, aborting   I'm guessing that will be a non-starter for this contribution. I'm also still wondering if you consider the current behavior to be a bug or not? The only alternative I can come up with is to add another choosing strategy that uses the new behavior so that people have a non-breaking migration path to the fix. Would you be interested in a contribution like that?

            People

            Assignee:
            rsandell rsandell
            Reporter:
            eisakson Eric Isakson
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Dates

              Created:
              Updated: