search

jenkinsci / build-timeout-plugin

Jenkins build-timeout plugin
https://plugins.jenkins.io/build-timeout/
31 stars 80 forks source link

Implemented review requests from pull request #44 #45

Closed Jochen-A-Fuerbacher closed 8 years ago

Jochen-A-Fuerbacher commented 9 years ago

Hi, I implemented the review requests from pull request 44 ( https://github.com/jenkinsci/build-timeout-plugin/pull/44 ).

Jochen

ikedam commented 9 years ago

Note from #44

ikedam commented 9 years ago

This indicates an option to fail the build if retries exceeded is useful.

ikedam commented 9 years ago

It makes sense that rescheduling the build only when a timeout occurs. That cannot be achieved with naginator-plugin. However, At the same time, it 's preferred to integrate rescheduling features into one plugin.

What we really need is an operation to have naginator-plugin reschedule the build, isn't it? That requires to extend naginator-plugin to accept commandes to reschedule from other plugins and I'll work for that. Would you let me have time for that work?

Continuing this pull request is useful as you can use this feature locally or naginator-plugin might not like that extension.

Jochen-A-Fuerbacher commented 9 years ago

@ikedam Yes, that would be nice.

ikedam commented 9 years ago

I sent a pull request to naginator-plugin. https://issues.jenkins-ci.org/browse/JENKINS-29715 https://github.com/jenkinsci/naginator-plugin/pull/16

ikedam commented 9 years ago

I found this feature requested long time ago: JENKINS-8947

ikedam commented 9 years ago

Sorry to have kept you waiting.

Now naginator-plugin-1.16alpha-1 is availeble, which allows us trigger naginator-plugin from build-timeout plugin. The outline to integrate:

@Jochen-A-Fuerbacher @stefanbrausch Can you continue this development? If so, I also want you to write unittests for this new feature.

Jochen-A-Fuerbacher commented 9 years ago

We will continue this development, but it could take a little bit time, because we have other projects at the moment with higher priority.

ikedam commented 9 years ago

No problem to take a time. Thanks in advance. Please let me know if you need any help.

Jochen-A-Fuerbacher commented 9 years ago

I implemented naginator integration in build-timeout. But I can't run unit tests of build-timeout plugin. The existing(!) tests fail. According to Maven output, it's a problem with JSP support. When I force Maven to only test my unit test, it's the same problem. I also figured out that my test fails when calling JenkinsRule. Do you have any advice?

ikedam commented 9 years ago

I don't think I can help you only with that information.

Jochen-A-Fuerbacher commented 9 years ago

Ok, I reduced my test to:

@Rule public BuildTimeOutJenkinsRule j = new BuildTimeOutJenkinsRule();

@Test @LocalData public void testAbortAndRestart() throws Exception { }

where testAbortAndRestart() doesn't contain anything.

When I force Maven to only run my "test" with -Dtest=AbortAndRestartOperationTest, I get the following error:

Tests run: 1, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 3.08 sec <<< FAILURE! testAbortAndRestart(hudson.plugins.build_timeout.operations.AbortAndRestartOperationTest) Time elapsed: 2.77 sec <<< ERROR! java.lang.NoClassDefFoundError: hudson/model/AbstractBuild$AbstractBuildExecution at java.lang.ClassLoader.defineClass1(Native Method) at java.lang.ClassLoader.defineClass(ClassLoader.java:791) at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142) at java.net.URLClassLoader.defineClass(URLClassLoader.java:449) at java.net.URLClassLoader.access$100(URLClassLoader.java:71) at java.net.URLClassLoader$1.run(URLClassLoader.java:361) at java.net.URLClassLoader$1.run(URLClassLoader.java:355) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:354) at java.lang.ClassLoader.loadClass(ClassLoader.java:423) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308) at java.lang.ClassLoader.loadClass(ClassLoader.java:356) at java.lang.Class.getDeclaredMethods0(Native Method) at java.lang.Class.privateGetDeclaredMethods(Class.java:2451) at java.lang.Class.getDeclaredMethods(Class.java:1810) at org.jvnet.hudson.annotation_indexer.Index$2$1.fetch(Index.java:101) at org.jvnet.hudson.annotation_indexer.Index$2$1.hasNext(Index.java:71) at org.jvnet.hudson.annotation_indexer.FilterIterator.fetch(FilterIterator.java:23) at org.jvnet.hudson.annotation_indexer.FilterIterator.hasNext(FilterIterator.java:42) at hudson.init.InitializerFinder.discoverTasks(InitializerFinder.java:70) at hudson.init.InitializerFinder.discoverTasks(InitializerFinder.java:55) at org.jvnet.hudson.reactor.TaskBuilder$2.discoverTasks(TaskBuilder.java:40) at org.jvnet.hudson.reactor.Reactor.(Reactor.java:128) at org.jvnet.hudson.reactor.Reactor.(Reactor.java:133) at jenkins.model.Jenkins$6.(Jenkins.java:823) at jenkins.model.Jenkins.executeReactor(Jenkins.java:823) at jenkins.model.Jenkins.(Jenkins.java:763) at hudson.model.Hudson.(Hudson.java:81) at org.jvnet.hudson.test.JenkinsRule.newHudson(JenkinsRule.java:486) at org.jvnet.hudson.test.JenkinsRule.before(JenkinsRule.java:312) at org.jvnet.hudson.test.JenkinsRule$1.evaluate(JenkinsRule.java:431) at org.junit.rules.RunRules.evaluate(RunRules.java:18) at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222) at org.junit.runners.ParentRunner.run(ParentRunner.java:300) at org.apache.maven.surefire.junit4.JUnit4TestSet.execute(JUnit4TestSet.java:53) at org.apache.maven.surefire.junit4.JUnit4Provider.executeTestSet(JUnit4Provider.java:123) at org.apache.maven.surefire.junit4.JUnit4Provider.invoke(JUnit4Provider.java:104) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:601) at org.apache.maven.surefire.util.ReflectionUtils.invokeMethodWithArray(ReflectionUtils.java:164) at org.apache.maven.surefire.booter.ProviderFactory$ProviderProxy.invoke(ProviderFactory.java:110) at org.apache.maven.surefire.booter.SurefireStarter.invokeProvider(SurefireStarter.java:172) at org.apache.maven.surefire.booter.SurefireStarter.runSuitesInProcessWhenForked(SurefireStarter.java:104) at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:70) Caused by: java.lang.ClassNotFoundException: hudson.model.AbstractBuild$AbstractBuildExecution at java.net.URLClassLoader$1.run(URLClassLoader.java:366) at java.net.URLClassLoader$1.run(URLClassLoader.java:355) at java.security.AccessController.doPrivileged(Native Method) at java.net.URLClassLoader.findClass(URLClassLoader.java:354) at java.lang.ClassLoader.loadClass(ClassLoader.java:423) at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:308) at java.lang.ClassLoader.loadClass(ClassLoader.java:356) ... 53 more

ikedam commented 9 years ago

AbstractBuild.AbstractBuildExecution isn't available in Jenkins 1.466: https://github.com/jenkinsci/jenkins/commit/14dbcd88d37c675baf6c58682519d129e45cb221 I'm not sure what caused that dependency. What happens if you removed LocalData?

ikedam commented 9 years ago

I suppose your current working branch is https://github.com/Jochen-A-Fuerbacher/build-timeout-plugin/tree/naginator :

Your trouble might be caused by the mismatch of the Jenkins versions of this plugin and of naginator plugin. If changing the target version resolves the problem, you can continue the development with that branch.

ikedam commented 9 years ago

I found that the problem reproduces also in my environment and I can avoid that in the following way:

    <dependency>
      <groupId>org.jenkins-ci.plugins</groupId>
      <artifactId>naginator</artifactId>
      <version>1.16-alpha-1</version>
      <optional>true</optional>
      <exclusions>
        <exclusion>
          <groupId>org.jenkins-ci.plugins</groupId>
          <artifactId>matrix-project</artifactId>
        </exclusion>
      </exclusions>
    </dependency>

Details:

Jochen-A-Fuerbacher commented 9 years ago

When I exclude matrix-project like you did, I get the following exception:

SEVERE: Failed Loading plugin naginator java.io.IOException: Dependency matrix-project (1.2) doesn't exist

My workaround is, that I changed the required core in pom.xml to 1.609.3. This is the actual LTS version of Jenkins. With that change all existing tests run fine.

ikedam commented 9 years ago

My workaround is, that I changed the required core in pom.xml to 1.609.3. This is the actual LTS version of Jenkins. With that change all existing tests run fine.

Ok. Please continue the development with that change.

I'll make a pull request for naginator-plugin not to depend on matrix-project plugin. It might be merged in naginator-plugin 1.16.

ikedam commented 9 years ago

Though this work may be still in progress, I added some comments that make it easy for you to write and run unit tests. Please let me know if this request is ready for full reviews.

ikedam commented 9 years ago

You can ignore errors from findbugs if raised in lines you didn't change. Newer Jenkins core are fully annotated with findbugs and the check gets more strict when upgrading the target version. Of course, you can fix them if you like it.

Jochen-A-Fuerbacher commented 9 years ago

I changed the code you commented. You can do a full review of this request.

Thank you!

ikedam commented 9 years ago

It's better to accept variable expressions for maxRestarts. Really sorry for a late request, but it will get hard to change a type of a field after making a release. You can expand variable with build.getEnvironment(listener).expand(string);. Would you try that?

Jochen-A-Fuerbacher commented 9 years ago

I changed maxRestarts to accept variable expressions.

ikedam commented 9 years ago

With the é in the name, I get encoding error when building.

Hmm...I'll have a look on that. I think I can resolve that by adding encoding instructions in pom.xml.

ikedam commented 9 years ago

Thanks for the update. I added a few more comments, but I think they can be fixed easily.

Please wait for a little more time till naginator 1.16 is released. I believe we can revert the target version after that and it makes many things easier (e.g. firebug no longer claims errors).

Jochen-A-Fuerbacher commented 8 years ago

Thank you for your comments. I changed the code you commented. Anything I should do because of the encoding problem?

ikedam commented 8 years ago

Anything I should do because of the encoding problem?

Nothing to do for now. We'll revert the target Jenkins version when the new version of naginator plugin is released, and we can revert that file at that time.

I'll change the encoding of that file in another commit (It's not a good idea to do that in this request).

ikedam commented 8 years ago

Sorry for having you wait for so long time! Naginator-plugin 1.16 was released. It may take some time till it's available in the update center, but should be already available in the maven repository.

Would you do followings?:

Jochen-A-Fuerbacher commented 8 years ago

Hello,

in the next few weeks I don't have the time to work on that plugin. But it's in my backlog and I'm looking forward to work on this as soon as possible.

ikedam commented 8 years ago

Rebased and recreated #54.

ikedam commented 8 years ago

Released in build-timeout 1.17.