jenkinsci / build-timeout-plugin

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

[FIXED JENKINS-29163] Added Deadline timeout strategy #39

Closed fmiguelez closed 9 years ago

fmiguelez commented 9 years ago

I added the functionality described in the ticket. It works, I tested it with multi-configuration projects and created a unit test.

Ikedam please review the pull request in case you have any suggestions before merging it.

ikedam commented 9 years ago

The functionality looks perfect.

ikedam commented 9 years ago

Accepting variables would be useful. E.g. You can configure the deadline globally with system variables.

ikedam commented 9 years ago

The time getTimeOut is called is after the build gets out from the build queue and after checking out SCM. If you don't like that, following APIs may help you.

ikedam commented 9 years ago

link to JIRA JENKINS-29163.

fmiguelez commented 9 years ago

Regarding using variables I will definetly include it.

fmiguelez commented 9 years ago

I close this pull request and recreate another one with the suggested improvements.

ikedam commented 9 years ago

JFYI: You can continue this request. Just pushing additional commits to your repository updates this request automatically. (in that case, github doesn't notify me the update and you have to comment that you make updates)

Of course, you can recreate a new request. You can do as you prefer.

fmiguelez commented 9 years ago

Ok I will do as you mention, I had no idea.

fmiguelez commented 9 years ago

ikedam, is it ok if I merge now the pull request?

ikedam commented 9 years ago

Many unrelated changes to BuildTimeOutStrategy. You can see your changes in the "Files changed" tab.

Let's turn off your IDE auto-formatting for existing codes, and don't make unrelated changes to existing codes as:

I prefer to leave existing codes unchanged even if I know they are formatted badly.

fmiguelez commented 9 years ago

Sorry for that. I usually let Eclipse do the formatting and imports organizing on save action. I think I fixed BuildTimeoutOutStrategy as you requested. Now only my changes at the bottom show up. I needed some trial and error (not really familiar with git yet :( ) .

ikedam commented 9 years ago

Looks good to me. Please let me merge this request manually, squashing your last some commits together.

This request contains commits confusing git blame like this: https://github.com/fmiguelez/build-timeout-plugin/blame/c04fab08ab89313e594eae8950939ec3cfc94fc0/src/main/java/hudson/plugins/build_timeout/BuildTimeOutStrategy.java (you can reach that page from "Files Changed" > "View" > "Blame")

This can be resolved by squashing commits after "[CHG]..." together using several git commands. Let me have time for that work.

ikedam commented 9 years ago

I rearranged commits in #40 (Preserves your commit comments, but SHA-1 is changed). Could you have a look on the request and see your change is preserved?

fmiguelez commented 9 years ago

Great work. Content matches in both pull requests. We can close this pull request and merge #40 then.