Add ability to cancel specific jobs with trigger scoped cancellation when building current patchset or other options similar to server scope. #415

MattLud · 2020-05-27T18:36:02Z

This PR will add the ability for specific jobs to have running builds aborted upon seeing a new patchset pushed. The options added mirror the server-level triggers that currently exist but are instead scoped to trigger that have the options enabled. This should not impact existing server cancellation.

… for current patchset updates

rsandell · 2020-05-29T12:28:14Z

build-config/checkstyle-suppressions.xml

@@ -11,4 +11,6 @@
              files="InjectedTest.java"/>
    <suppress checks=".+"
              files="[/\\]generated-sources[/\\]"/>
+    <suppress checks="FileLength"


#392 (comment)

I don't mind refactoring - my plan is to move that parts of that subclass logic to it's own file. Any concerns with that?

Went ahead and did this.

rsandell

I haven't reviewed everything yet.

rsandell · 2020-05-29T12:37:39Z

src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java

@@ -794,7 +798,8 @@ public ListBoxModel doFillVerdictCategoryItems() {
    public void notifyBuildEnded(GerritTriggeredEvent event) {
        if (event instanceof ChangeBasedEvent) {
            IGerritHudsonTriggerConfig serverConfig = getServerConfig(event);
-            if (serverConfig != null && serverConfig.isGerritBuildCurrentPatchesOnly()) {
+            if ((serverConfig != null && serverConfig.isGerritBuildCurrentPatchesOnly())
+                    || this.getBuildCancellationPolicy().isEnabled()) {


You check for null in EventListener so I guess you need to check for it here as well.

rsandell · 2020-05-29T12:42:02Z

src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTrigger.java

+    /**
+     * @return the abortNewPatchsets
+     */
+    public boolean isAbortNewPatchsets() {


Instead of a whole bunch of new fields I'd prefer to just have one bean with the setting in. It can be achieved with for example <f:block> or rather <f:optionalBlock>

Will try that out again. My last attempt doing that jelly code ended in tears.

Still no success. How would I rewrite the Jelly + java code to make this work?

rsandell · 2020-05-29T12:58:24Z

src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/EventListener.java

-            if (serverConfig != null && serverConfig.isGerritBuildCurrentPatchesOnly()) {
+            if (t.getBuildCancellationPolicy() != null && t.getBuildCancellationPolicy().isEnabled())
+            {
+                t.getRunningJobs().cancelTriggeredJob(changeBasedEvent,


I might be wrong, but it looks strange; since you aren't calling scheduled here I don't think that it is recorded among the running jobs correctly and so can't be cancelled?

I'll double check - the code to keep it separate from cancelling ALL jobs with the triggered event was a bit more nuanced.

So if we don't have a serverwide or trigger policy, it doesn't get added.

If the trigger policy is set, we add it here: https://github.com/jenkinsci/gerrit-trigger-plugin/pull/415/files#diff-23a1e696752ef9408f31a5c9aa77c5d3R2408

It was moved when refactored - https://github.com/jenkinsci/gerrit-trigger-plugin/pull/415/files#diff-da694feab949be34e2a85c791e48f0fbR118

rsandell · 2020-05-29T13:03:30Z

...st/java/com/sonyericsson/hudson/plugins/gerrit/trigger/spec/SpecGerritTriggerHudsonTest.java

@@ -91,6 +91,8 @@
    private SshServer sshd;
    private SshdServerMock serverMock;
    private GerritServer gerritServer;
+    private static final int DELAY = 2000;


I think you had to do this because you got outside of the area for MagicNumber ignore rule?
You could just increase the number on Line 73.

I don't mind moving away from that magical number unless you want it there. Either way I can revert and update the checkstyle or remove it entirely.

…policy Change-Id: I1e654243a9030635062339f3657f71eb1580ee73

Change-Id: I12c65e99c10d8df714e52157fe730fe519e737a1

MattLud · 2020-07-06T21:08:41Z

I think all that's left on this is the Jelly code rewrite - is the anything else you need fixed?

rsandell · 2020-09-25T13:28:11Z

src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/RunningJobs.java

+     * Class for maintaining and synchronizing the runningJobs info.
+     * Association between patches and the jobs that we're running for them.
+     */
+    public class RunningJobs {


You missed to reformat.

rsandell · 2020-09-25T14:55:56Z

Looks good.
Here is an example of how to make the jelly work. It could probably be made a bit cleaner, maybe by using f:optionalProperty and other things, but at least it should work for you to experiment further with.

Change-Id: Ibc27ad7b252ec0762f677a32ce32ab62a1b8958a

MattLud · 2020-10-08T17:35:54Z

Reworking this PR now to fix tests. Should have an update either today or tomorrow.

rsandell · 2020-10-09T14:30:03Z

And I've probably introduced a merge conflict hell for you today, sorry about that :/

MattLud · 2020-10-09T16:25:55Z

No worries - I'm tracking down a regression with RunningJobs being moved out anyways.

Change-Id: I208713390358c780587fa7c03ecae96ebfa8c197

…plugin into trigger_cancel_current_patchset Change-Id: I0b201c254f758b88b4bd34448d53ae1338f5a734

MattLud · 2020-10-09T21:49:07Z

Pushed new change to branch but this PR isn't updated. May need to poke github to see if there is a sync issue.

Change-Id: I93543b86a3fbd7c84bb9adb28d5a1b9d9755a2db

MattLud · 2020-10-12T16:19:05Z

@rsandell - Ok ready for review. Lemme know if you want me to rebase/squash commits.

MattLud · 2020-10-23T14:34:16Z

@rsandell - Not sure what happened on that last check - can it be re-run?

rsandell · 2020-11-20T14:45:07Z

@MattLud could you change the title to something more descriptive, for the changelog, and add a description please?

MattLud · 2020-11-25T19:02:14Z

@MattLud could you change the title to something more descriptive, for the changelog, and add a description please?

Will do! Expect it later today - Thanks again for reviewing it!

romanek-adam · 2020-12-16T11:26:53Z

@MattLud could you maybe provide a specific use case this change addresses? I'd like to understand the rationale and potentially improve on our side by taking advantage of it.

MattLud · 2020-12-17T18:58:28Z

@romanek-adam Previously, you could only set server-wide setting if a build would be aborted if a new patchet set was published.

Since we run multi-tenant jenkins servers with a variety of needs, we needed granularity to control this at the trigger/job level.

ctudor · 2021-01-06T18:31:22Z

Any reason this change removed my fix for only storing events for the specified gerrit server? Running into the same issues I had before with the latest plugin where we keep replaying events that should not be replayed.

9d95d15#diff-d20ef7cffb03100f9595b376cd9f40692a8b0788597e87830df09fa0f3244a63

MattLud added 2 commits May 8, 2020 14:42

Add option for individual triggers to cancel builds for their project…

173b2d9

… for current patchset updates

Fixing stylechecks

e795f3e

rsandell reviewed May 29, 2020

View reviewed changes

rsandell requested changes May 29, 2020

View reviewed changes

MattLud added 2 commits June 4, 2020 12:55

refactor RunningJobs out of GerritTrigger; add null check on trigger …

14a0765

…policy Change-Id: I1e654243a9030635062339f3657f71eb1580ee73

Fixing checkstyle errors

a109b02

Change-Id: I12c65e99c10d8df714e52157fe730fe519e737a1

rsandell reviewed Sep 25, 2020

View reviewed changes

MattLud and others added 3 commits October 7, 2020 15:49

Fixing format of new file

483ce2f

Change-Id: Ibc27ad7b252ec0762f677a32ce32ab62a1b8958a

Remove unneeded buildPlugin configuration

76180a8

Jelly property bean example

926668d

MattLud added 2 commits October 9, 2020 16:14

Refactor to use proper jelly code

bba5ecb

Change-Id: I208713390358c780587fa7c03ecae96ebfa8c197

Merge branch 'master' of https://github.com/jenkinsci/gerrit-trigger-…

1773d55

…plugin into trigger_cancel_current_patchset Change-Id: I0b201c254f758b88b4bd34448d53ae1338f5a734

Removing set timeout for trigger test

93f3399

Change-Id: I93543b86a3fbd7c84bb9adb28d5a1b9d9755a2db

rsandell approved these changes Nov 20, 2020

View reviewed changes

rsandell added the enhancement label Nov 20, 2020

MattLud changed the title ~~Trigger cancel current patchset~~ Added ability to cancel specific jobs with trigger scoped cancellation when building current patchset or other options similar to server scope. Dec 1, 2020

rsandell merged commit f182f42 into jenkinsci:master Dec 2, 2020

MattLud deleted the trigger_cancel_current_patchset branch December 3, 2020 05:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to cancel specific jobs with trigger scoped cancellation when building current patchset or other options similar to server scope. #415

Add ability to cancel specific jobs with trigger scoped cancellation when building current patchset or other options similar to server scope. #415

MattLud commented May 27, 2020 •

edited

rsandell May 29, 2020

MattLud Jun 1, 2020 •

edited

MattLud Jun 4, 2020

rsandell left a comment

rsandell May 29, 2020

MattLud Jun 4, 2020

rsandell May 29, 2020

MattLud Jun 1, 2020

MattLud Jul 6, 2020

rsandell May 29, 2020

MattLud Jun 1, 2020

MattLud Jun 4, 2020

rsandell Sep 25, 2020

MattLud Oct 7, 2020

rsandell May 29, 2020

MattLud Oct 7, 2020

MattLud commented Jul 6, 2020

rsandell Sep 25, 2020

rsandell commented Sep 25, 2020 •

edited

MattLud commented Oct 8, 2020

rsandell commented Oct 9, 2020

MattLud commented Oct 9, 2020

MattLud commented Oct 9, 2020

MattLud commented Oct 12, 2020 •

edited

MattLud commented Oct 23, 2020

rsandell commented Nov 20, 2020

MattLud commented Nov 25, 2020

romanek-adam commented Dec 16, 2020

MattLud commented Dec 17, 2020

ctudor commented Jan 6, 2021

Add ability to cancel specific jobs with trigger scoped cancellation when building current patchset or other options similar to server scope. #415

Add ability to cancel specific jobs with trigger scoped cancellation when building current patchset or other options similar to server scope. #415

Conversation

MattLud commented May 27, 2020 • edited

Choose a reason for hiding this comment

MattLud Jun 1, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MattLud commented Jul 6, 2020

Choose a reason for hiding this comment

rsandell commented Sep 25, 2020 • edited

MattLud commented Oct 8, 2020

rsandell commented Oct 9, 2020

MattLud commented Oct 9, 2020

MattLud commented Oct 9, 2020

MattLud commented Oct 12, 2020 • edited

MattLud commented Oct 23, 2020

rsandell commented Nov 20, 2020

MattLud commented Nov 25, 2020

romanek-adam commented Dec 16, 2020

MattLud commented Dec 17, 2020

ctudor commented Jan 6, 2021

MattLud commented May 27, 2020 •

edited

MattLud Jun 1, 2020 •

edited

rsandell commented Sep 25, 2020 •

edited

MattLud commented Oct 12, 2020 •

edited