• Icon: Bug Bug
    • Resolution: Unresolved
    • Icon: Major Major
    • None
    • Jenkins ver. 2.150.3
      GitHub Branch Source Plugin 2.4.2

      I have a GitHub Organisation configured and it is operating fairly satisfactorily.  It finds PRs when they are opened and builds them, and so on, quite successfully.

      But every now and then it will build not only the PR but also the branch that the PR is on, adding a continuous-integration/jenkins/branch commit status to the PR.

      This is wasteful in the least a can be confusing to the PR submitters about why they have this strange new commit status occasionally.

      I suppose this could happen legitimately if there is latency between pushing to a new branch on GitHub and opening the PR for it.  Is there any delay on building new branches to account for this sort of behaviour?  If not, perhaps there should be?

          [JENKINS-56255] occasionally builds the branch for a PR also

          Brian J Murrell added a comment - - edited

          What about "Ignore change requests flagged as originating from an untrusted source"? It is in your image above.

          My understanding of that option is that it doesn't actually prevent Jenkins from building from the untrusted source but rather just instructs Jenkins to ignore the Jenkinsfile such a change-request and use the Jenkinsfile from the project's master.

          The above was my confusing the new feature you are suggesting using with some previous functionality around PRs from untrusted sources.

          I agree, but my point was I think the problem is inherent in the feature. How would you redesign this to make it not flawed? Adding a delay would be mean that all branches wait a certain amount of time before they are built, which is generally not desirable.

          How frequent is creating new branches that are not backed by a PR in the real world? Because this delay should only really be required by new branches given that subsequent pushes to the branch already have the PR in place to tell Jenkins not to build the branch. And if it were configurable from 0 to whatever delay a Jenkins admin desires, then it's backward compatible with existing (flawed) behaviour of anyone wants to retain that. But it does allow fixing this behaviour for anyone who desires that.

          Let's get those gripers together and discuss how to address the problem. 

          The gripers are griping about a completely different issue to this issue so it's really quite orthogonal. They are griping about an issue that occurs when trying to employ your suggested "work-around". And the explanation of why your work-around cannot work in some situations is only use-case why that work-around doesn't work. It could just plainly be that an organisation doesn't want PRs from forks for other organisational reasons.

          Ultimately any workarounds are just pussy-footing around the real issue of the branch/PR race that is inherent in any design which doesn't allow for a (configurable if you wish) delay to give the PR time to be submitted. So please, let's not get off into the weeds about why any particular work-around is not suitable and just fix the root issue?

          Brian J Murrell added a comment - - edited What about "Ignore change requests flagged as originating from an untrusted source"? It is in your image above. My understanding of that option is that it doesn't actually prevent Jenkins from building from the untrusted source but rather just instructs Jenkins to ignore the Jenkinsfile such a change-request and use the Jenkinsfile from the project's master. The above was my confusing the new feature you are suggesting using with some previous functionality around PRs from untrusted sources. I agree, but my point was I think the problem is inherent in the feature. How would you redesign this to make it not flawed? Adding a delay would be mean that all branches wait a certain amount of time before they are built, which is generally not desirable. How frequent is creating new branches that are not backed by a PR in the real world? Because this delay should only really be required by new branches given that subsequent pushes to the branch already have the PR in place to tell Jenkins not to build the branch. And if it were configurable from 0 to whatever delay a Jenkins admin desires, then it's backward compatible with existing (flawed) behaviour of anyone wants to retain that. But it does allow fixing this behaviour for anyone who desires that. Let's get those gripers together and discuss how to address the problem.  The gripers are griping about a completely different issue to this issue so it's really quite orthogonal. They are griping about an issue that occurs when trying to employ your suggested "work-around". And the explanation of why your work-around cannot work in some situations is only use-case why that work-around doesn't work. It could just plainly be that an organisation doesn't want PRs from forks for other organisational reasons. Ultimately any workarounds are just pussy-footing around the real issue of the branch/PR race that is inherent in any design which doesn't allow for a (configurable if you wish) delay to give the PR time to be submitted. So please, let's not get off into the weeds about why any particular work-around is not suitable and just fix the root issue?

          Liam Newman added a comment - - edited

          brianjmurrell

          It sounds like you're getting frustrated with my responses. I want to be clear that I am not trying to block you, I am trying to help.

          I want to make sure you (and anyone else encountering this issue) have a reasonable work-around. It sound like we've achieved that using branch filtering - either by script added to the pipeline or by branch filter setting added to the job configuration. The more involved work-around is to move to the more mainstream pattern of doing PRs from forks (and using "Ignore change requests flagged as originating from an untrusted source" if you need to prevent building of PRs from any rando forks).

          I'm not blocking anyone from implementing a fix for this. I'd like that fix to make sense and be useful, so I'm asking questions about what it should look like. I apologize if that came off as too skeptical or pessimistic. Let me try again.

          The fix you proposed in seems like a reasonable improvement, which also has some limitations. I'd like to make sure that when someone goes to implement this they understand those limitations as part of the proposed behavior.

          I'm one of several regular contributors to this plugin but to my knowledge most us are already at full capacity currently. Perhaps you or someone from your organization could implement this?

          Liam Newman added a comment - - edited brianjmurrell It sounds like you're getting frustrated with my responses. I want to be clear that I am not trying to block you, I am trying to help. I want to make sure you (and anyone else encountering this issue) have a reasonable work-around. It sound like we've achieved that using branch filtering - either by script added to the pipeline or by branch filter setting added to the job configuration. The more involved work-around is to move to the more mainstream pattern of doing PRs from forks (and using "Ignore change requests flagged as originating from an untrusted source" if you need to prevent building of PRs from any rando forks). I'm not blocking anyone from implementing a fix for this. I'd like that fix to make sense and be useful, so I'm asking questions about what it should look like. I apologize if that came off as too skeptical or pessimistic. Let me try again. The fix you proposed in seems like a reasonable improvement, which also has some limitations. I'd like to make sure that when someone goes to implement this they understand those limitations as part of the proposed behavior. I'm one of several regular contributors to this plugin but to my knowledge most us are already at full capacity currently. Perhaps you or someone from your organization could implement this?

          bitwiseman I tested your work-around,  and have verified that the Ignore change requests flagged as originating from an untrusted source option (recently added I think) feature does as it seems and as such could be a valid work-around. But that doesn't make our current workflow any less valid[1] and it might be that I cannot convince all of my users to switch to submitting from forks now that they have gotten used to the branch-in-organisation workflow.

          [1] One redeeming feature of the workflow of using branches inside the main repo is that everyone can easily be aware of all of all WIP simply by inspecting the branches of repo.

          Brian J Murrell added a comment - bitwiseman I tested your work-around,  and have verified that the Ignore change requests flagged as originating from an untrusted source option (recently added I think) feature does as it seems and as such could be a valid work-around. But that doesn't make our current workflow any less valid [1] and it might be that I cannot convince all of my users to switch to submitting from forks now that they have gotten used to the branch-in-organisation workflow. [1] One redeeming feature of the workflow of using branches inside the main repo is that everyone can easily be aware of all of all WIP simply by inspecting the branches of repo.

          Liam Newman added a comment -

          brianjmurrell
          Hey, thanks for testing that alternative.
          And yes, I totally agree branch-in-organization is a totally a valid workflow, in which case the branch filtering work-around is the way to go.

          Liam Newman added a comment - brianjmurrell Hey, thanks for testing that alternative. And yes, I totally agree branch-in-organization is a totally a valid workflow, in which case the branch filtering work-around is the way to go.

          But having to do branch filtering does not embrace the principle of least surprise.

          I am sure most people will find it surprising that the branch/PR race design is not more robust and that they have to employ branch filtering to augment it.

          On the other hand, I think a configuration item, right next to (or below) the selector for whether you want to build branches that are not PRs – for how long to delay a first-time branch build to decide if it's going to become a PR is really straightforward.  I think most people would understand that delay is how Jenkins decides if branches are going to become PRs.  Not having anything at all leaves people to not even consider the design problem and assuming that Jenkins will then just figure it out.  And then it doesn't.

          Brian J Murrell added a comment - But having to do branch filtering does not embrace the principle of least surprise. I am sure most people will find it surprising that the branch/PR race design is not more robust and that they have to employ branch filtering to augment it. On the other hand, I think a configuration item, right next to (or below) the selector for whether you want to build branches that are not PRs – for how long to delay a first-time branch build to decide if it's going to become a PR is really straightforward.  I think most people would understand that delay is how Jenkins decides if branches are going to become PRs.  Not having anything at all leaves people to not even consider the design problem and assuming that Jenkins will then just figure it out.  And then it doesn't.

          OK.  So I am back.

          There is something more flawed than just a race between new branch and PR discovery going on here.

          This doesn't just happen on initial branch/PR creation (i.e Build #1) but happens on subsequent pushes to the PR/branch.  If this were just a race, subsequent pushes would not fall victim to this, correct?

          Additionally, working from forks (as much as my users really want to do it) is actually, unworkable due to these bugs.

          But working from forks or not is actually quite orthogonal, as I have described above, this does not seem to merely be a race condition on new branches/forks.  This doesn't seem to be "by design".  This seems like an actual bug.

          Brian J Murrell added a comment - OK.  So I am back. There is something more flawed than just a race between new branch and PR discovery going on here. This doesn't just happen on initial branch/PR creation (i.e Build #1) but happens on subsequent pushes to the PR/branch.  If this were just a race, subsequent pushes would not fall victim to this, correct? Additionally, working from forks (as much as my users really want to do it) is actually, unworkable due to these bugs. But working from forks or not is actually quite orthogonal, as I have described above, this does not seem to merely be a race condition on new branches/forks.  This doesn't seem to be "by design".  This seems like an actual bug.

          Roman Pickl added a comment -

          I have the same issue.

          I have the feeling it is even worse. the started branch builds (with multiple stages/agents) seem to fail as jenkins already cleans up after detecting a PR:

          Roman Pickl added a comment - I have the same issue. I have the feeling it is even worse. the started branch builds (with multiple stages/agents) seem to fail as jenkins already cleans up after detecting a PR:

          Roman Pickl added a comment -

          brianjmurrell does the workaround in https://issues.jenkins-ci.org/browse/JENKINS-58442 "fix" this for you?
          I'm interested, because I've implemented it as well and it looks ok so far.

          Roman Pickl added a comment - brianjmurrell does the workaround in https://issues.jenkins-ci.org/browse/JENKINS-58442 "fix" this for you? I'm interested, because I've implemented it as well and it looks ok so far.

          rompic No unfortunately not, per my previous comment.

          Brian J Murrell added a comment - rompic No unfortunately not, per my previous comment .

          Wade added a comment -

          > How frequent is creating new branches that are not backed by a PR in the real world? Because this delay should only really be required by new branches given that subsequent pushes to the branch already have the PR in place to tell Jenkins not to build the branch.

          I do this all of the time once I've opened a PR and am making revisions against it; if I have a branch `feature` that is a PR then I'll push to `feature-staging`, verify that CI passes, then merge `feature-staging` into `feature`.  This prevents me from accidentally pushing totally broken stuff to the original PR.

           

          That being said, I've also been running into issues because of the current behavior.  The most nefarious issue is when an intermittent failure causes the branch build to fail but the PR build succeeds; this causes the overall PR on GitHub to be marked as failing CI because of the branch build failure.  A workaround is to manually re-trigger the build in the branch and hope it succeeds, but if the branch has been cleaned up there's no way to even re-trigger the branch build so the PR stays marked as failing CI with no way to fix the GitHub status from Jenkins.  There should be some kind of way to (preferably automatically) detect and fix this issue from Jenkins.

          In my opinion, the following two things should be done when a PR build is created and the corresponding branch is supposed to be excluded from the build:

          1. Abort any outstanding build jobs against the branch job
          2. Create a pending build status for the PR and then submit a successful build status against the branch (since there doesn't appear to be a way to clear existing statuses)

          This would allow branch jobs to continue as normal while also doing the needed clean-up when a PR is opened.

          In addition, when scanning a GitHub repo, Jenkins should check for PRs with stale build statuses and fix them as above.

          Thoughts?

          P.S. The workaround to fix a stale branch build status is:

          $ curl -L -X POST -H "Authorization: Bearer ${TOKEN}" https://api.github.com/repos/${OWNER}/${REPO}/statuses/${SHA} -d '{"state":"success","description":"Manual fix","context":"continuous-integration/jenkins/branch"}' 

          Wade added a comment - > How frequent is creating new branches that are not backed by a PR in the real world? Because this delay should only really be required by new branches given that subsequent pushes to the branch already have the PR in place to tell Jenkins not to build the branch. I do this all of the time once I've opened a PR and am making revisions against it; if I have a branch `feature` that is a PR then I'll push to `feature-staging`, verify that CI passes, then merge `feature-staging` into `feature`.  This prevents me from accidentally pushing totally broken stuff to the original PR.   That being said, I've also been running into issues because of the current behavior.  The most nefarious issue is when an intermittent failure causes the branch build to fail but the PR build succeeds ; this causes the overall PR on GitHub to be marked as failing CI because of the branch build failure.  A workaround is to manually re-trigger the build in the branch and hope it succeeds, but if the branch has been cleaned up there's no way to even re-trigger the branch build so the PR stays marked as failing CI with no way to fix the GitHub status from Jenkins .  There should be some kind of way to (preferably automatically) detect and fix this issue from Jenkins. In my opinion, the following two things should be done when a PR build is created and the corresponding branch is supposed to be excluded from the build: Abort any outstanding build jobs against the branch job Create a pending build status for the PR and then submit a successful build status against the branch (since there doesn't appear to be a way to clear existing statuses) This would allow branch jobs to continue as normal while also doing the needed clean-up when a PR is opened. In addition, when scanning a GitHub repo, Jenkins should check for PRs with stale build statuses and fix them as above. Thoughts? P.S. The workaround to fix a stale branch build status is: $ curl -L -X POST -H "Authorization: Bearer ${TOKEN}" https: //api.github.com/repos/${OWNER}/${REPO}/statuses/${SHA} -d '{ "state" : "success" , "description" : "Manual fix" , "context" : "continuous-integration/jenkins/branch" }'

            Unassigned Unassigned
            brianjmurrell Brian J Murrell
            Votes:
            2 Vote for this issue
            Watchers:
            4 Start watching this issue

              Created:
              Updated: