JENKINS-65075 Update PluginPatchsetCreatedEvent shouldTriggerOn #428
Conversation
When a Jenkins job is configured with a PatchSet Created, and the "commit message contains regular expression" is populated, using the "Query and Trigger Gerrit Patches" link found at <jenkins_url>/gerrit_manual_trigger/ will incorrectly trigger the job. Update the PatchSetCreatedEvent shouldTriggerOn function to check for the commit message before inspecting if it's a ManualPatchSetCreated event. Issue: JENKINS-65075
| this.commitMessageContainsRegEx, Pattern.DOTALL | Pattern.MULTILINE); | ||
| } | ||
| String commitMessage = ((PatchsetCreated)event).getChange().getCommitMessage(); | ||
| return commitMessagePattern.matcher(commitMessage).find(); |
There was a problem hiding this comment.
Returning this early will will nullify all the other exclude checks below
There was a problem hiding this comment.
True. I didn't think of that.
In my opinion, the ManualPatchsetCreated event should behave just like a PatchsetCreated event. That is, the two triggers should be identical. it's just that one allow you to retrigger jobs without having to actually create a new Gerrit patch set.
If that is the case, can't we just remove the event instanced ManualPatchsetCreated block?
There was a problem hiding this comment.
As I see it, it works as intended.
A better fix would be to allow a custom commit message to be provided when manually triggering and then remove the check forevent instanceof ManualPatchsetCreated.
I don't think allow a custom commit message to be provided when manually triggering the right way of doing this as the commit message should already be available (it should be the commit message of the current patch set).
It is possible for the ManualPatchsetCreated to contain t he same information as the PatchsetCreated event?
Any thoughts on how to proceed with this?
| this.commitMessageContainsRegEx, Pattern.DOTALL | Pattern.MULTILINE); | ||
| } | ||
| String commitMessage = ((PatchsetCreated)event).getChange().getCommitMessage(); | ||
| return commitMessagePattern.matcher(commitMessage).find(); |
There was a problem hiding this comment.
True. I didn't think of that.
In my opinion, the ManualPatchsetCreated event should behave just like a PatchsetCreated event. That is, the two triggers should be identical. it's just that one allow you to retrigger jobs without having to actually create a new Gerrit patch set.
If that is the case, can't we just remove the event instanced ManualPatchsetCreated block?
|
Yea, I didn't think of that. IIRC you can now get the full message from the query. It could be that before you couldn't get that info or we chose to not include it in the search result in fear of requesting too much data. |
When a Jenkins job is configured with a PatchSet Created, and the
"commit message contains regular expression" is populated, using the
"Query and Trigger Gerrit Patches" link found at
<jenkins_url>/gerrit_manual_trigger/ will incorrectly trigger the job.
Update the PatchSetCreatedEvent shouldTriggerOn function to check for
the commit message before inspecting if it's a ManualPatchSetCreated
event.
Issue: JENKINS-65075