KostyaSha / github-integration-plugin

Jenkins GitHub Integration Plugin
https://wiki.jenkins-ci.org/display/JENKINS/GitHub+Integration+Plugin
MIT License
97 stars 84 forks source link

Skip + commit changed still triggers a build #56

Closed mikz closed 8 years ago

mikz commented 8 years ago

I have a pr that was build, even though it should not. Using the Label not exists trigger configured to skip PRs with missing label jenkins. That was correctly skipped as the log shows. But then I have trigger to build PR when commit changed and that triggered a build. But it should not, because the PR was skipped.

Running GitHub Pull Request trigger check for Mar 10, 2016 12:58:22 PM on 3scale/system
GitHub rate limit before check: GHRateLimit{remaining=4205, limit=5000, resetDate=Thu Mar 10 13:01:51 CET 2016}
Labels not exist: [jenkins] not found
Skipping PR #6469
GitHubPRCommitEvent: new commit found, sha 984050ce9facfd8ea772bd1acd9649ee713284f8
Prepare to trigger build for PR #6469, because Commit changed
Finished GitHub Pull Request trigger check for 3scale/system at Mar 10, 2016 12:58:26 PM. Duration: 3540ms

Any ideas?

KostyaSha commented 8 years ago

Have no ideas :atm: Try check that Skipping PR #6469 really returns true for skippable in it's cause or what it returns in cause object.

mikz commented 8 years ago

Looks like Skipping PR really returns true/false. https://github.com/jenkinsci/github-integration-plugin/blob/f45e15c495855adcaa6a0e976749be57b7fa99b5/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/trigger/check/SkippedCauseFilter.java#L18-L26

Looks like it it used there https://github.com/jenkinsci/github-integration-plugin/blob/6c1a9124050684ed529a80e333b0693c07656582/github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/trigger/check/PullRequestToCauseConverter.java#L46-L51

But I really have no idea what this should do. But it looks like the triggers are executed in several passes so the skip does not apply.

KostyaSha commented 8 years ago

Try add logger for org.jenkinsci.plugins.github.pullrequest. Logic was the next:

Could you wrote the test with two causes and mocked data?

mikz commented 8 years ago

I have 0 experience with Java development, so I'm not really comfortable doing that.

But I just added the logger and will try to reproduce and report what I find.

mikz commented 8 years ago

Here is the log:

Mar 10, 2016 3:19:52 PM FINE org.jenkinsci.plugins.github.pullrequest.webhook.GHPullRequestSubscriber onEvent
Queued check for system-pipeline (PR #6473) after heavy hook
Mar 10, 2016 3:19:52 PM FINE org.jenkinsci.plugins.github.pullrequest.utils.LoggingTaskListenerWrapper debug
Running GitHub Pull Request trigger check for Mar 10, 2016 3:19:52 PM on 3scale/system
Mar 10, 2016 3:19:52 PM FINE org.jenkinsci.plugins.github.pullrequest.utils.LoggingTaskListenerWrapper debug
GitHub rate limit before check: GHRateLimit{remaining=4668, limit=5000, resetDate=Thu Mar 10 16:01:51 CET 2016}
Mar 10, 2016 3:19:53 PM INFO org.jenkinsci.plugins.github.pullrequest.trigger.check.NotUpdatedPRFilter isUpdated
Pull request #6473 was updated at: Thu Mar 10 15:17:29 CET 2016 by mikz
Mar 10, 2016 3:19:54 PM FINE org.jenkinsci.plugins.github.pullrequest.events.impl.GitHubPRLabelNotExistsEvent check
Labels not exist:[jenkins] not found
Mar 10, 2016 3:19:54 PM FINE org.jenkinsci.plugins.github.pullrequest.utils.LoggingTaskListenerWrapper debug
Skipping PR #6473
Mar 10, 2016 3:19:54 PM FINE org.jenkinsci.plugins.github.pullrequest.events.impl.GitHubPRCommitEvent check
New commit. Sha: ef92b341f493957607924ad7e45a9e45d3348664 => 1db0d04be4c3e6ea7cc96710c128e81f2f7d406c
Mar 10, 2016 3:19:55 PM FINE org.jenkinsci.plugins.github.pullrequest.utils.LoggingTaskListenerWrapper debug
Prepare to trigger build for PR #6473, because Commit changed
Mar 10, 2016 3:19:55 PM FINEST org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger readyToBuildCauses
Causes count for 3scale/system: 1
Mar 10, 2016 3:19:56 PM INFO org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger readyToBuildCauses
GitHub rate limit after check 3scale/system: GHRateLimit{remaining=4652, limit=5000, resetDate=Thu Mar 10 16:01:51 CET 2016}, consumed: 16, checked PRs: 1
Mar 10, 2016 3:19:56 PM INFO org.jenkinsci.plugins.github.pullrequest.utils.LoggingTaskListenerWrapper info
Finished GitHub Pull Request trigger check for 3scale/system at Mar 10, 2016 3:19:56 PM. Duration: 3788ms
Mar 10, 2016 3:19:56 PM INFO org.jenkinsci.plugins.github.pullrequest.trigger.JobRunnerForCause apply
Jenkins queued the run (Commit changed)

with relevant pool log:

Running GitHub Pull Request trigger check for Mar 10, 2016 3:19:52 PM on 3scale/system
GitHub rate limit before check: GHRateLimit{remaining=4668, limit=5000, resetDate=Thu Mar 10 16:01:51 CET 2016}
Labels not exist: [jenkins] not found
Skipping PR #6473
GitHubPRCommitEvent: new commit found, sha 1db0d04be4c3e6ea7cc96710c128e81f2f7d406c
Prepare to trigger build for PR #6473, because Commit changed
Finished GitHub Pull Request trigger check for 3scale/system at Mar 10, 2016 3:19:56 PM. Duration: 3788ms
KostyaSha commented 8 years ago

@onecloud-sean found the reason :) It was regression after migration to streams.

mikz commented 8 years ago

Thank you so much for fixing it! Compiled the latest HEAD and looks like it works as is supposed to.

KostyaSha commented 8 years ago

Will do release as soon as pass my integration tests :)

KostyaSha commented 8 years ago

Released, should appear in update center tomorrow.