Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: don't trigger poll unless target ref matches #270

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dnwe
Copy link

@dnwe dnwe commented May 25, 2022

The existing webhook handling code would trigger the GitSCM poll for the default branch regardless of whether or not the target ref of the webhook push event matched the branch specifier of the Job definition. This is problematic and often leads to duplicate builds for the same branch+revision if pushes to other branches happened to occur whilst build(s) were already in progress for that branch. This is because the GitSCM trigger just compares the HEAD revision of any branch matching the branch specifier against the last completed build of that branch (ignoring in-progress builds) and schedules a new build for it.

The code for this PR is mostly a rebased version of the original changes proposed by @aserrallerios under #44 with some small refinements.

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@dnwe
Copy link
Author

dnwe commented May 25, 2022

Note: there is likely still discussion and changes needed here, but I wanted to open a PR to re-open discussions as the old one had long been abandoned and closed.

@dnwe dnwe force-pushed the only-trigger-if-branchspec-matches-ref branch 5 times, most recently from 94da2c1 to a954bb3 Compare May 31, 2022 21:28
@dnwe dnwe force-pushed the only-trigger-if-branchspec-matches-ref branch from a954bb3 to 743c2c1 Compare June 6, 2022 10:49
@dnwe dnwe force-pushed the only-trigger-if-branchspec-matches-ref branch from 743c2c1 to f8eb872 Compare March 3, 2023 09:36
@dnwe dnwe force-pushed the only-trigger-if-branchspec-matches-ref branch 4 times, most recently from 8709832 to 16bbbbd Compare March 23, 2023 09:53
@dnwe
Copy link
Author

dnwe commented Apr 27, 2023

@KostyaSha would you be willing to take a look over this PR? It does fix the problem and has been running in our CI for a while now

@dnwe
Copy link
Author

dnwe commented Jan 16, 2024

@oleg-nenashev have you taken over as a maintainer on github-plugin now, please could you take a look at this PR?

@KostyaSha
Copy link
Member

KostyaSha commented Jan 16, 2024

@KostyaSha would you be willing to take a look over this PR? It does fix the problem and has been running in our CI for a while now

That looks like out of scope of current functionality, current functionality is only kick poll. For native branch triggering there is https://github.com/KostyaSha/github-integration-plugin which deals with exact branches and doesn't use gitscm.

@dnwe
Copy link
Author

dnwe commented Jan 16, 2024

As per the original description from 2 years ago, the intention of this PR is not to provide native branch triggering, the intention is to prevent pushes to unrelated branches triggering a build.

i.e., if the Jenkins build is configured to build the default branch of the repo (e.g., normally master or main) then the build should only be triggered when a GitHub push happens to that branch, not when one happens to other branches within the repository.

@dnwe dnwe force-pushed the only-trigger-if-branchspec-matches-ref branch from 16bbbbd to 069e927 Compare January 22, 2024 10:48
dnwe and others added 4 commits March 21, 2024 17:17
The existing webhook handling code would trigger the GitSCM poll for the
default branch regardless of whether or not the target ref of the
webhook push event matched the branch specifier of the Job definition.
This is problematic and often leads to duplicate builds for the same
branch+revision if pushes to other branches happened to occur whilst
build(s) were already in progress for that branch. This is because the
GitSCM trigger just compares the HEAD revision of any branch matching
the branch specifier against the last *completed* build of that branch
(ignoring in-progress builds) and schedules a new build for it.

The code for this PR is mostly a rebased version of the original changes
proposed by @aserrallerios under
jenkinsci#44 with some small
refinements.

Co-authored-by: Albert Serrallé Ríos <[email protected]>
@dnwe dnwe force-pushed the only-trigger-if-branchspec-matches-ref branch from 069e927 to 6efaba0 Compare March 21, 2024 17:18
@jakubgs
Copy link

jakubgs commented Apr 26, 2024

I don't know why this PR is stuck but this functionality is definitely needed. Recently we started seeing infinite build loops in our website builds due to GitHub SCM Polling triggering both when master is pushed and when gh-pages is pushed by the job itself, resulting in an infinite loop.

@KostyaSha this definitely is an essential functionality, and it does seem to me like it used to work, since this infinite loop behavior is something I only started observing in our setup a few months ago. I don't know what changed but this PR certainly appears to be fixing it. Please reconsider, saying that this is "out of scope" is a bit silly to me, and it should to you if you think about it a bit.

If I'm configuring my branch to fetch master branch and enable "GitHub hook trigger for GITScm polling" I obviously intend the builds to run for that branch, and not any branch.

@jakubgs
Copy link

jakubgs commented Apr 26, 2024

@dnwe
Copy link
Author

dnwe commented Apr 26, 2024

@jakubgs that other plugin doesn’t use this plugin, so that linked issue isn’t related

But yes, this PR would prevent your gh-pages pushed from triggering a new master branch build (causing endless loop). We’ve run the .hpi from the build of this PR for more than 2 years now
(to fix the bug) without issue and the change itself is very small and is fixing a bug not adding a feature, so I’m not sure why it couldnt be reviewed and merged

@jakubgs
Copy link

jakubgs commented Apr 26, 2024

Thanks for the contribution man. I will try it, because just yesterday Jenkins committed 6000 times to our gh-pages branch for one website. I don't understand what changed since this used to work, but it makes no sense. Git poll log states:

 > git ls-remote -h -- [email protected]:status-im/infra-bi.git # timeout=10
Found 15 remote heads on [email protected]:status-im/infra-bi.git
[poll] Latest remote head revision on refs/heads/master is: d88fecd7ef67995f14d7f2163a01dc5decabe5ae - already built by 615

So the build was already performed for given commit, and then it just runs again for no good reason. I don't understand it.

Will try your fix. Thanks.

@jakubgs
Copy link

jakubgs commented Apr 26, 2024

Though I'm not fan of running custom builds as they will not receive fixes for vulnerabilities and other bugs.

But it is better than running infinite website build loops...

@jakubgs
Copy link

jakubgs commented Apr 26, 2024

I actually built the HPI from this project using mvn hpi:hpi, but now the "GitHub hook trigger for GITScm polling" button is gone:

image

Is there some special flag that needs to be used at build time that I don't know about?

@jakubgs
Copy link

jakubgs commented Apr 26, 2024

I wasn't really on dnwe:only-trigger-if-branchspec-matches-ref branch, it just defaulted to master, despite that branch being the default for the fork. But I built dnwe:only-trigger-if-branchspec-matches-ref and installed it through "advanced" page in plugins section, but the "GitHub hook trigger for GITScm polling" button is still missing, any ideas?

@dnwe
Copy link
Author

dnwe commented Apr 26, 2024

@jakubgs sounds like your compile didn't work — just grab this .hpi file from the linked Jenkins jobs on the PR.

Direct Link: github-1.38.1-rc825.6efa_b_a_0b_7b_f8.hpi

@jakubgs
Copy link

jakubgs commented Apr 26, 2024

It doesn't seem to work, builds are still triggered by other branches:

image

This job is configured to build from develop, and yet is being triggered by deploy-develop. This issue is infuriating.

@dnwe
Copy link
Author

dnwe commented Apr 26, 2024

@jakubgs make sure your branch specifier is absolute with the refs/heads prefix

image

^ here's one of mine (for example), make yours refs/heads/develop if that's the only branch you want to build off

@jakubgs
Copy link

jakubgs commented Apr 28, 2024

Thanks for the help, but it doesn't seem this branch fixes the loop. My website builds still get stuck in an endless build loop.

Need to debug this more.

@yakimant
Copy link

In our case this was an issue with git-plugin for shared libraries polling.
Polling was incorrectly deciding on branch and commit calculation.

Switching to github-plugin solved the issue.

As of this PR, it was causing these log messages, not sure how harmful they were:

May 14, 2024 3:04:19 PM FINE org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventSubscriber$1 run

Considering to poke SOME_JOB

May 14, 2024 3:04:19 PM SEVERE org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventSubscriber$1 run

parseAssociatedBranches threw exception
java.lang.IllegalArgumentException: The specified method is not declared by the specified base class (com.cloudbees.jenkins.GitHubRepositoryNameContributor), or it is private, static or final.
    at hudson.Util.isOverridden(Util.java:1535)
    at com.cloudbees.jenkins.GitHubRepositoryNameContributor.parseAssociatedBranches(GitHubRepositoryNameContributor.java:99)
    at com.cloudbees.jenkins.GitHubRepositoryNameContributor.parseAssociatedBranches(GitHubRepositoryNameContributor.java:173)
    at org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventSubscriber$1.run(DefaultPushGHEventSubscriber.java:127)
    at hudson.security.ACL.impersonate2(ACL.java:425)
    at hudson.security.ACL.impersonate(ACL.java:437)
    at org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventSubscriber.onEvent(DefaultPushGHEventSubscriber.java:159)

...

May 14, 2024 3:04:19 PM INFO org.jenkinsci.plugins.github.webhook.subscriber.DefaultPushGHEventSubscriber$1 run

Poked SOME_JOB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants