Skip to content

Conversation

@aserrallerios
Copy link

Review on Reviewable

@cloudbees-pull-request-builder

plugins » github-plugin #45 SUCCESS
This pull request looks good

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@dagolden
Copy link

dagolden commented Aug 5, 2014

This would be a really nice feature. I hope someone can review it and merge it soon.

@lptr
Copy link

lptr commented Sep 25, 2014

+1 this would be very useful to us as well.

@oleg-nenashev
Copy link
Member

The PR conflicts with a current master branch => could you rebase it?
It would be also great to have an issue on Jenkins JIRA

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Javadoc is required.
Is there any risk of regressions in other plugins (if somebody declares a GithubTrigger)?

@oleg-nenashev
Copy link
Member

@aserrallerios
The request should be merged into the current master branch.
Could you also add severalunit tests fro the new functionality?

@aserrallerios
Copy link
Author

Rebased, and decoupled some concepts. Added some tests for the GitHub - SMC branch matching.

I tried to add some tests for the GitHubWebHook code but I was unable to mock everything up.

@oleg-nenashev oleg-nenashev added this to the github-1.11 milestone Jan 27, 2015
@jstriebel
Copy link

+1 really missing this feature at the moment!

@freeyoung
Copy link

+1 for this feature

@ema
Copy link

ema commented Feb 10, 2015

+1 please

1 similar comment
@tavisto
Copy link

tavisto commented Feb 10, 2015

+1 please

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exclude new line

@KostyaSha KostyaSha modified the milestones: 1.12, github-1.11 Feb 25, 2015
@yani-
Copy link

yani- commented Feb 26, 2015

+1 please

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you coding from mobile? Why so short width?

@npaufler
Copy link

Am I understanding this PR correctly in that it will filter the branch out of the github webhook push request, so that only the branch that has received a push will be poked? We have a serious issue right now whereby we effectively DoS ourselves due to the number of branches we have and a webhook push causes -every- branch to get considered and poked.

If so, any idea when this will get merged in?

I did try building it locally (installed oracle JDK7, maven, and did a mvn hpi:hpi per the readme) but the resulting plugin .hpi file did not seem to work - Jenkins did not recognize it correctly. All github references were removed from the config options, and Jenkins complained about incorrect data in the config files.

I think it's more likely that I made a mistake in building it than a problem with the PR, though. Happy to try it if I can get a pointer on how to properly build it.

@oleg-nenashev
Copy link
Member

@npaufler
The change is valid and useful, but the implementation needs an improvement: binary backward compatibility, usage of classes from github-api library. Somebody just needs to invest some time and to rework the pull request.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why it hardcoded? What if i want trigger refs/tags, refs/notes?

@KostyaSha
Copy link
Member

@KostyaSha
Copy link
Member

@npaufler

We have a serious issue right now whereby we effectively DoS ourselves due to the number of branches we have and a webhook push causes -every- branch to get considered and poked.

What do you mean under "-every- branch to get considered and poked"?
I have some ideas what can be improved for performance but firstly i want understand other use cases

In general this PR looks not well formed, @aserrallerios proposed PR without referring to any jira issue or any description.

@kohsuke
Copy link
Member

kohsuke commented Apr 27, 2015

It's quite possible that I'm not understanding this, but if you are interested in scheduling builds only for specific branches, the right thing to do is to set the branch spec in the Git SCM configuration.

All this plugin is doing is to schedule a polling, and it's up to the Git plugin to determine if the change pushed is worth the build.

There are lots of criteria when it comes to what is considered an interesting change and what is not. We can't add every one of them in this plugin. We've learned that lesson in the Subversion plugin and the Git v1 plugin.

@KostyaSha
Copy link
Member

@kohsuke probably penalty is in heavy weight polling check that is called in SCM internals. Of course github plugin can't determine that some job configuration meets build criteria (for example excluded messages and other behaviours), but it can do light filtering, for example by filtering not matched branch specifier configuration. (just a guess)

@KostyaSha
Copy link
Member

Btw, @aserrallerios @npaufler could you test this PR #39 ? It changes the way how plugin triggers builds it also deal with branches. I think #39 will be merged in any case because it switches to better API usage.

@KostyaSha KostyaSha removed this from the github-1.12 milestone Apr 27, 2015
@llasram
Copy link

llasram commented Nov 1, 2015

I'm interested in the feature this PR provides, but it seems fairly complex and inflexible vs the approach of providing values from the push event payload as build parameters, as discussed in JENKINS-24291.

I hacked together enough a prototype to verify that exposing such parameters is both doable and enables the results that I'm trying to achieve. If I cleaned the implementation and opened a PR, would such a change be accepted?

@KostyaSha
Copy link
Member

@llasram i doubt. Atm trigger do the simple thing - it calls GitSCM poll that then checks whether build should be scheduled. Gitscm contains branch specifier and other features that "filter" == choses branch to build. If you need "filter" branch, then set correct branch specified in gitscm configuration.

@KostyaSha
Copy link
Member

Closing as initial author didn't respond to any questions and code is not mergeable anymore for a long time.

@KostyaSha KostyaSha closed this Nov 2, 2015
@talcloudshare
Copy link

Is there any intention on making this work for upcoming versions? Seems like a really useful functionality

@hachi
Copy link

hachi commented Apr 18, 2017

Can we please get a working version of this? There are several tickets that have been open since 2014 that apply to this concept.

Passing data from the webhook as environment variables would be minimally useful.
Filtering branches or commits automatically by a checkbox would be the ultimate.

Either way, having a new project that builds all branches because it has never been built before is pretty useless. I just hit this at work and had a 20minute build job that fired off for some 30 branches because of the number of engineers working... and a brand new jenkins installation.

@KostyaSha
Copy link
Member

KostyaSha commented Apr 18, 2017

YOU CAN'T DO IT BECAUSE PUSH TRIGGER IS ONLY KICKS POLLING WITH SEPARATE LOGIC.
That written in documentation. If you want achieve what you want, you need to use other triggers.

dnwe added a commit to dnwe/github-plugin that referenced this pull request 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
jenkinsci#44 with some small
refinements.

Co-authored-by: Albert Serrallé Ríos <[email protected]>
dnwe added a commit to dnwe/github-plugin that referenced this pull request May 30, 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
jenkinsci#44 with some small
refinements.

Co-authored-by: Albert Serrallé Ríos <[email protected]>
dnwe added a commit to dnwe/github-plugin that referenced this pull request Mar 3, 2023
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 added a commit to dnwe/github-plugin that referenced this pull request Mar 22, 2023
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 added a commit to dnwe/github-plugin that referenced this pull request Mar 22, 2023
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 added a commit to dnwe/github-plugin that referenced this pull request Mar 22, 2023
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 added a commit to dnwe/github-plugin that referenced this pull request Mar 23, 2023
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 added a commit to dnwe/github-plugin that referenced this pull request Jan 22, 2024
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 added a commit to dnwe/github-plugin that referenced this pull request Mar 21, 2024
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 added a commit to dnwe/github-plugin that referenced this pull request Aug 20, 2024
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 added a commit to dnwe/github-plugin that referenced this pull request Nov 8, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.