Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-26323] Fix "Build Current Patches Only" by scanning for event #195

Merged
merged 2 commits into from Jan 9, 2015

Conversation

Projects
None yet
5 participants
@HedAurabesh
Copy link
Contributor

commented Jan 7, 2015

The "Build Current Patches Only" feature currently identifies previous
builds based on their parameters as they were present when the build was
first scheduled.

If you use a plugin that alters the parameters after the job has started,
the cancelling of previous builds does not work, since the
GerritTrigger#cancelJob() method does not find the job anymore.

This is easily and trivially fixed, by not scanning for the parameters, but
instead for the GerritEvent that is stored in the GerritCause used to start
a build.

The only downside to this is, that the current patch-set scans for identity,
but not equality of the two events. After a Jenkins restart, they might not
be identical anymore, since Jenkins reschedules queued builds and can't
unify the two events via Stapler; as they are stored in separate files.

It'd be trivially easy to alter this to Event.equals(otherEvent), in case
this degradation is not wanted.

[EDIT]
Duplicate of pull request #194. That one was aborted, since it targeted an outdated release tag.

[JENKINS-26323] Fix "Build Current Patches Only" by scanning for event
The "Build Current Patches Only" feature currently identifies previous
builds based on their parameters as they were present when the build was
first scheduled.

If you use a plugin that alters the parameters after the job has started,
the cancelling of previous builds does not work, since the
GerritTrigger#cancelJob() method does not find the job anymore.

This is easily and trivially fixed, by not scanning for the parameters, but
instead for the GerritEvent that is stored in the GerritCause used to start
a build.

The only downside to this is, that the current patch-set scans for identity,
but not equality of the two events. After a Jenkins restart, they might not
be identical anymore, since Jenkins reschedules queued builds and can't
unify the two events via Stapler; as they are stored in separate files.

It'd be trivially easy to alter this to Event.equals(otherEvent), in case
this degradation is not wanted.
@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Jan 7, 2015

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

@rinrinne

This comment has been minimized.

Copy link
Member

commented Jan 8, 2015

Did you test with manual trigger?

if (param.equals(parameters)) {
Queue.getInstance().cancel(item);
}
List<hudson.model.Queue.Item> itemsInQueue = Queue.getInstance().getItems(myProject);

This comment has been minimized.

Copy link
@rsandell

rsandell Jan 8, 2015

Member

myProject doesn't exist anymore since the last merged pull request.
Use getJob or job instead. Hence the build failure.

This comment has been minimized.

Copy link
@HedAurabesh

HedAurabesh Jan 8, 2015

Author Contributor

Thanks, will fix it ASAP. That's what you get for re-basing and just looking at the mergetool. :P

Fixed removal of "myProject" member
Also, switched from Hudson.getInstance() to Jenkins.getInstance(), as the
former is marked as deprecated in newer Jenkins versions.
@HedAurabesh

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2015

@rinrinne Yes, I tested it. Manual triggers always abort all previous runs started for that change, no matter which patch-set was restarted or was previously running. It also does not matter, whether you restart via the special "manual trigger" page or via the "Retrigger / Retrigger All" page in the side panel. All previous runs get immediately aborted.

As far as I can tell, this is the same behaviour as in the previous implementation, where it checked the used parameters for equality.

Of course, manual runs and runs based on other change IDs are completely unaffected and continue running normally.

As a sidenote: I have no clue why the Jenkins test failed:
https://jenkins.ci.cloudbees.com/job/plugins/job/gerrit-trigger-plugin/392/

From my limited point of view, it does not look like it's related to the changes in this commit (especially because only one single test case failed).

@scoheb

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2015

Hi,

I had the same test failure happen to me while preparing a PR myself. This was going to be my fix:

@@ -102,15 +103,11 @@ public class BuildCompletedRestCommandJobHudsonTest {

         PluginImpl.getInstance().getHandler().post(event);

-        //CS IGNORE MagicNumber FOR NEXT 9 LINES. REASON: TestData
-        long startedWait = System.currentTimeMillis();
-        while (project.getLastCompletedBuild() == null) {
-            if (System.currentTimeMillis() - startedWait > 10000) {
-                throw new RuntimeException("Timeout!");
-            }
-            System.out.println("waiting for build to complete...");
-            Thread.sleep(1000);
-        }
+        TestUtils.waitForBuilds(project, 1);
+
+        //CS IGNORE MagicNumber FOR NEXT 3 LINES. REASON: TestData
+        System.out.println("waiting for post build notification...");
+        Thread.sleep(5000);

It seems it is a timing issue. We cannot just simply wait for the build to be completed...We also need to wait until the post build notification is received. Hence, the additional "sleep".

Hope that helps!

Scott

@rsandell

This comment has been minimized.

Copy link
Member

commented Jan 9, 2015

Thanks @scoheb I'm currently debugging some flaky tests and this was one of them.
I'll push it directly onto master if that's ok.

rsandell added a commit that referenced this pull request Jan 9, 2015

Merge pull request #195 from HedAurabesh/v2.13.0-abort-on-new-fix
[JENKINS-26323] Fix "Build Current Patches Only" by scanning for event

@rsandell rsandell merged commit 2c0a523 into jenkinsci:master Jan 9, 2015

0 of 2 checks passed

Jenkins Looks like there's a problem with this pull request
Details
default Jenkins is validating pull request ...
Details
@scoheb

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2015

By all means @rsandell !

Thanks

Scott

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.