fixes intermittent multiple job invocations #41
Conversation
fixes intermittent multiple job invocatino
|
@lprimak, thanks for the contribution! Interesting that this is causing the problems, but I can see how possibly it could trigger a job twice if two SCM owners were attached to it and they both match. However, what happens here when there are legitimately multiple SCM sources that point at the same repository? In our Jenkins we have multiple jobs pointing at the same GitHub source repository. This changes plugin behavior in a way that would break the functionality for that use case I believe. Can you provide more details on how this fixes the problem? |
|
There is only one SCM owner. The issue is intermittent. I can see where a subtle cases could have the legitimate second job not being triggered. I do understand this change may not be acceptable. Do you have another suggestion? |
|
@bluesliverx what do you think of above? thanks! |
|
Well, I'm honestly not sure how this fixes it, but I'm willing to try it. If it ends up causing issues in our build system though (such as missed triggers), I'll need to revert it. But let's give it a shot :) |
|
Thanks! |
| @@ -161,13 +162,14 @@ public void run() { | |||
| changedRepository.getRepositoryName() | |||
| } | |||
| ); | |||
| break topLevel; | |||
There was a problem hiding this comment.
Sadly this has broken the following workflow, in which we expect multiple builds to be triggered:
- we have multiple jobs which each define
triggerPRCommentBranchPropertye.g. one withcommentBody(".*foo this please.*")and another withcommentBody(".*bar this please.*") - it was previously possible to comment on a PR like this and both jobs would be triggered:
foo this please
bar this please
- however now only 1 of them is triggered (whichever one the
for (Job...loop gets to first)
The workaround is of course to do a separate comment for each one, but when you want to trigger 5 different custom test runs, this is a 5x increase in (a) effort to initiate, and (b) the amount of email spam sent to others watching the PR.
Perhaps if instead of doing break it instead checked whether job had already been scheduled, that would be a way of avoiding duplicates without stopping multiple different jobs being scheduled from the same comment?
There was a problem hiding this comment.
We have been seeing issues for weeks with similar problems, and your comment just made me connect it to this PR. I'm downgrading the plugin on our Jenkins to verify that this is indeed the cause.
There was a problem hiding this comment.
@sparrowt please open a new issue.
As I mentioned in the comment #41 (comment),
this would break the cases where multiple legitimate jobs would be triggered by a single comment.
Unfortunately I don't have the environment to test this on.
This PR fixes the 99% case where this plugin was broken for a long time, as reported in multiple issues by multiple people.
I can propose a short-term fix by adding a checkbox that turns off one-per-comment build (default would be on) since that's 99% of the cases.
In the future if there is more interest to see what actually causes duplicate builds maybe there would be more time to take this on
There was a problem hiding this comment.
Sorry I never got round to this - however it looks like #49 was opened and #48 fixes it (released in https://github.com/jenkinsci/github-pr-comment-build-plugin/releases/tag/82.v986d7874d9c5) so thank you!
|
@bluesliverx please don't revert this. It breaks the 99% cases for this plugin. |
|
@lprimak sorry I've been out due to some personal things at home. I'm not planning on reverting this necessarily. I was saying that I downgraded the version that was installed on our Jenkins cluster to see if it helped, and it did indeed, so this is definitely the troublesome commit. I like the solution proposed by someone else, look at the queue to see if it was already scheduled for the job to prevent multiple starts for a single job. |
Fixes #32 and #27