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

GitHub commit status context not unique enough or configurable - jobs always overwrite each other

    • Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Critical Critical
    • None
    • CloudBees Jenkins Enterprise 2.107.34.0.1-fixed
      GitHub API Plugin 1.90
      GitHub Branch Source Plugin 2.3.5

      GitHub can incorrectly flag PRs as passing checks if multiple jobs run against the same repository.

      Multiple jobs configured against the same source repo will incorrectly set all checks successful if the last completed check is successful.

      In my opinion, if there is no mechanism to define mandatory and optional checks, then the worst status check should provide the overall result.

      This could be a check prior to submitting status that validates the prior status is not unsuccessful, and  it is the same git sha1, and it is a different job name.

      The secondary issue here is that only a single link is added to the PR, and so there is no indication on the GitHub PR how many jobs ran.

          [JENKINS-55726] GitHub commit status context not unique enough or configurable - jobs always overwrite each other

          Related to JENKINS-37100 and JENKINS-36574 - I think the latter may be the genesis for the current situation, which may have been a fix by jglick?

          I read through those bugs and can definitely see why the code is as it currently is, but I think an improvement can be made to satisfy both the current use case and also improve the situation where multiple Jenkins jobs are reporting to the single repository.

          I saw this mentioned but no discussion as to why it was ultimately not followed, so if there are issues then please do explain.

          The prime requirements for blocking merges is that there is a single string that any PR presents to GitHub so the repository settings can indicate the status of that context must be success prior to merge.

          This is currently set to "continuous-integration/jenkins/pr-merge".

           

              public String getDefaultContext(TaskListener listener) {
                  if (head instanceof PullRequestSCMHead) {
                      if (((PullRequestSCMHead) head).isMerge()) {
                          return "continuous-integration/jenkins/pr-merge";
                      } else {
                          return "continuous-integration/jenkins/pr-head";
                      }
                  } else {
                      return "continuous-integration/jenkins/branch";
                  }
              }

           

          The issue is that every Jenkins job returns that same context.

          In the case of a Multibranch configuration using GitHub, we will have a job whose name is [<folder>/]*<job>/<branch> where <branch> is either a branch or a special branch representing a Pull Request.

          The branch string changes each time a new branch or pull request is created, and this makes it unsuitable to be used as a merge gate for GitHub.  I would state that the enclosing job name is perfectly viable to use as a status context though, as that is unchanging across multiple Pull Requests.

          So the above code would become something like:

           

              public String getDefaultContext(TaskListener listener) {
                  if (head instanceof PullRequestSCMHead) {
                      if (((PullRequestSCMHead) head).isMerge()) {
                          return "continuous-integration/jenkins/" + job.getParent() + "/pr-merge";
                      } else {
                          return "continuous-integration/jenkins/" + job.getParent() + "/pr-head";
                      }
                  } else {
                      return "continuous-integration/jenkins/" + job.getParent() + "/branch";
                  }
              }
          

           

          Jon-Paul Sullivan added a comment - Related to  JENKINS-37100  and JENKINS-36574  - I think the latter may be the genesis for the current situation, which may have been a fix by jglick ? I read through those bugs and can definitely see why the code is as it currently is, but I think an improvement can be made to satisfy both the current use case and also improve the situation where multiple Jenkins jobs are reporting to the single repository. I saw this mentioned but no discussion as to why it was ultimately not followed, so if there are issues then please do explain. The prime requirements for blocking merges is that there is a single string that any PR presents to GitHub so the repository settings can indicate the status of that context must be success prior to merge. This is currently set to "continuous-integration/jenkins/pr-merge".   public String getDefaultContext(TaskListener listener) { if (head instanceof PullRequestSCMHead) { if (((PullRequestSCMHead) head).isMerge()) { return "continuous-integration/jenkins/pr-merge" ; } else { return "continuous-integration/jenkins/pr-head" ; } } else { return "continuous-integration/jenkins/branch" ; } }   The issue is that every Jenkins job returns that same context. In the case of a Multibranch configuration using GitHub, we will have a job whose name is [<folder>/] *<job>/<branch> where <branch> is either a branch or a special branch representing a Pull Request. The branch string changes each time a new branch or pull request is created, and this makes it unsuitable to be used as a merge gate for GitHub.  I would state that the enclosing job name is perfectly viable to use as a status context though, as that is unchanging across multiple Pull Requests. So the above code would become something like:   public String getDefaultContext(TaskListener listener) { if (head instanceof PullRequestSCMHead) { if (((PullRequestSCMHead) head).isMerge()) { return "continuous-integration/jenkins/" + job.getParent() + "/pr-merge" ; } else { return "continuous-integration/jenkins/" + job.getParent() + "/pr-head" ; } } else { return "continuous-integration/jenkins/" + job.getParent() + "/branch" ; } }  

          Jesse Glick added a comment -

          The context originally included the job name. This broke things badly for people who configured various tools according to specific contexts, so we switched to constant context names. The intention is that each commit is built by one job. If you want to define additional contexts, if I recall correctly there is a plugin which allows you to override the context name for a given job. There is also a plugin offering an explicit Pipeline step for adding an ad-hoc commit status.

          I think this can be closed or just treated as a request for clearer documentation.

          Jesse Glick added a comment - The context originally included the job name. This broke things badly for people who configured various tools according to specific contexts, so we switched to constant context names. The intention is that each commit is built by one job. If you want to define additional contexts, if I recall correctly there is a plugin which allows you to override the context name for a given job. There is also a plugin offering an explicit Pipeline step for adding an ad-hoc commit status. I think this can be closed or just treated as a request for clearer documentation.

          My reading was that the changing name returned from a PR branch build broke things because you need a fixed string for a single job.  I didn't read that as requiring a fixed string for every status reported by Jenkins ever.

          So ci/jenkins/repo/PR-1, ci/jenkins/repo/PR-2 == broken status reporting, but ci/jenkins/repo would not change between Pull Requests and therefore satisfies the requirements of the previous bug report, as I read it.

          If there is no desire to change the current default behaviour, I would still like a convenient way to change that default behaviour to something more useful to my use case, where multiple jobs need to accurately report status to GitHub without overwriting the reported status of a different job, which I think is a bug, even if the code is working as designed.

          I think this is a bug because it allows for incorrect changing of data in GitHub - different jobs reporting to the same context looks to me like avoiding the whole point of the context string in the first place.  So I would interpret the Jenkins intention is to have 1 job per commit, but that is not a model enforced by a number of other Jenkins<->SCM reporting tools, for example Gerrit and Jenkins allows for multiple jobs reporting status and correctly gates across multiple jobs.

          Requiring me to remember to change the context every time I create a uniquely named job seems like at best a poor user experience, and similarly requiring code in every Jenkinsfiles I write to correctly report back is again poor user experience.

          The additions need every time a job is created becomes tribal knowledge that needs to be remembered, and is going to be a source of bug in pipelines for our software delivery.

          Can I get pointers to the existing elements you mention though please?  They may show a template for creating a plugin that modifies the default behaviour?

          Jon-Paul Sullivan added a comment - My reading was that the changing name returned from a PR branch build broke things because you need a fixed string for a single job.  I didn't read that as requiring a fixed string for every status reported by Jenkins ever. So ci/jenkins/repo/PR-1, ci/jenkins/repo/PR-2 == broken status reporting, but ci/jenkins/repo would not change between Pull Requests and therefore satisfies the requirements of the previous bug report, as I read it. If there is no desire to change the current default behaviour, I would still like a convenient way to change that default behaviour to something more useful to my use case, where multiple jobs need to accurately report status to GitHub without overwriting the reported status of a different job, which I think is a bug, even if the code is working as designed. I think this is a bug because it allows for incorrect changing of data in GitHub - different jobs reporting to the same context looks to me like avoiding the whole point of the context string in the first place.  So I would interpret the Jenkins intention is to have 1 job per commit, but that is not a model enforced by a number of other Jenkins<->SCM reporting tools, for example Gerrit and Jenkins allows for multiple jobs reporting status and correctly gates across multiple jobs. Requiring me to remember to change the context every time I create a uniquely named job seems like at best a poor user experience, and similarly requiring code in every Jenkinsfiles I write to correctly report back is again poor user experience. The additions need every time a job is created becomes tribal knowledge that needs to be remembered, and is going to be a source of bug in pipelines for our software delivery. Can I get pointers to the existing elements you mention though please?  They may show a template for creating a plugin that modifies the default behaviour?

          Jesse Glick added a comment -

          Sorry, will leave detailed responses to whomever currently maintains the GitHub integration.

          Jesse Glick added a comment - Sorry, will leave detailed responses to whomever currently maintains the GitHub integration.

          Liam Newman added a comment -

          j3p0uk
          there is this plugin that lets people notify github in whatever way they choose.
          https://github.com/jenkinsci/pipeline-githubnotify-step-plugin

          Generally, the assumption for Pipeline has been that there is one pipeline per repository, so it was safe to have a hard-coded context. Are you not using Pipeline or using more than one pipeline per repository?

          In any case, having the github-branch-source support a user configurable context string seems like a reasonable feature request. Would you be willing to submit a PR for this?

          Liam Newman added a comment - j3p0uk there is this plugin that lets people notify github in whatever way they choose. https://github.com/jenkinsci/pipeline-githubnotify-step-plugin Generally, the assumption for Pipeline has been that there is one pipeline per repository, so it was safe to have a hard-coded context. Are you not using Pipeline or using more than one pipeline per repository? In any case, having the github-branch-source support a user configurable context string seems like a reasonable feature request. Would you be willing to submit a PR for this?

          Liam Newman added a comment - - edited

          FYI - Rejoice everyone!
          This functionality is available here:
          "Github Custom Notification Context SCM Behaviour"
          https://github.com/jenkinsci/github-scm-trait-notification-context-plugin

          Liam Newman added a comment - - edited FYI - Rejoice everyone! This functionality is available here: "Github Custom Notification Context SCM Behaviour" https://github.com/jenkinsci/github-scm-trait-notification-context-plugin

          Liam Newman added a comment -

          j3p0uk
          I see you changed this to in-progress. What else needs to be done here?

          Liam Newman added a comment - j3p0uk I see you changed this to in-progress. What else needs to be done here?

          bitwiseman, I did not change it, but I do think that the default behaviour of the plugin should not be one that allows information loss.

          Jon-Paul Sullivan added a comment - bitwiseman , I did not change it, but I do think that the default behaviour of the plugin should not be one that allows information loss.

          Liam Newman added a comment -

          j3p0uk
          What I meant was that JIRA says you changed this issue to "In Progress" and I wanted to know if you were working on it. If not, I'll change it back to open.

          Liam Newman added a comment - j3p0uk What I meant was that JIRA says you changed this issue to "In Progress" and I wanted to know if you were working on it. If not, I'll change it back to open.

          nozomu_honda - as you changed the status to in progress, could you answer bitwiseman’s question please?

          Jon-Paul Sullivan added a comment - nozomu_honda - as you changed the status to in progress, could you answer bitwiseman ’s question please?

          Nozomu Honda added a comment -

          bitwiseman j3p0uk

          Sorry for my late reply.

          I changed the status unintentionally. 

          Please change it back to open.

           

           

          Nozomu Honda added a comment - bitwiseman   j3p0uk Sorry for my late reply. I changed the status unintentionally.  Please change it back to open.    

          Pascal Hofmann added a comment - - edited

          https://github.com/jenkinsci/github-scm-trait-notification-context-plugin works like a charm for me with multi branch pipelines. It is also configurable via job-dsl:

          // Defines a custom context label to be sent as part of Github Status notifications for this project.
          notificationContextTrait {
              // The text of the context label for Github status notifications.
              contextLabel(String value)
              // Appends the relevant suffix to the context label based on the build type.
              typeSuffix(boolean value)
          }

          Pascal Hofmann added a comment - - edited https://github.com/jenkinsci/github-scm-trait-notification-context-plugin  works like a charm for me with multi branch pipelines. It is also configurable via job-dsl: // Defines a custom context label to be sent as part of Github Status notifications for this project. notificationContextTrait {     // The text of the context label for Github status notifications.     contextLabel (String value)     // Appends the relevant suffix to the context label based on the build type.     typeSuffix (boolean value) }

          bitwiseman, I finished rejoicing, but I'm not sure I'm ready to close this yet.

          I guess my main concern is that the default behaviour of Jenkins is one that allows loss of data and incorrect code merges.

          I would like to see can the default string be a safe alternative based on the job name in some way, rather than relying on either only a single job per repository, or the user knowing of the weakness and knowing to get the extra plugin to configure each of their jobs appropriately.

          Jon-Paul Sullivan added a comment - bitwiseman , I finished rejoicing, but I'm not sure I'm ready to close this yet. I guess my main concern is that the default behaviour of Jenkins is one that allows loss of data and incorrect code merges. I would like to see can the default string be a safe alternative based on the job name in some way, rather than relying on either only a single job per repository, or the user knowing of the weakness and knowing to get the extra plugin to configure each of their jobs appropriately.

            Unassigned Unassigned
            j3p0uk Jon-Paul Sullivan
            Votes:
            1 Vote for this issue
            Watchers:
            9 Start watching this issue

              Created:
              Updated: