[FIXED JENKINS-30564]

[fbelzunc]

timeoutMinutesElasticDefault should be used until numberOfBuilds is not reached out.

https://issues.jenkins-ci.org/browse/JENKINS-30564

@reviewbybees

[ghost]

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

[daniel-beck]

:bug: PR does not update existing relevant help texts to point out this special behavior.

Besides that implementation issue, this change does not have a firm rationale. It assumes a user configures elastic build timeout before the job even reached a stable behavior and execution time, which makes no sense.

OTOH users with aggressive deletion rules and/or frequently failing builds may be impacted by this change although they'd like the existing behavior: If you only keep 20 builds around due to e.g. storage limitations, but would like the elastic timeout to consider all of those that were successful, this change will break your existing timeout logic you rely on.

[fbelzunc]

PR does not update existing relevant help texts to point out this special behavior.

To do this is not a problem. It can be easily implemented.

Besides that implementation issue, this change does not have a firm rationale. It assumes a user configures elastic build timeout before the job even reached a stable behaviour and execution time, which makes no sense.

Is not there a similar assumption done with the standard behaviour when there is not already a successful build?

OTOH users with aggressive deletion rules and/or frequently failing builds may be impacted by this change although they'd like the existing behavior: If you only keep 20 builds around due to e.g. storage limitations, but would like the elastic timeout to consider all of those that were successful, this change will break your existing timeout logic you rely on.

For me this is the work timeoutMinutesElasticDefault will be doing. You should set-up the right value here to avoid any issue.

I understand it might have an impact, but I think this change is what an user which start with this plugin will expect. IMHO the current behaviour is not really expected unless you dig into the code.

[daniel-beck]

@fbelzunc To clarify, the :bug: is about the incomplete implementation only. The second and third paragraph stand alone as comment. Although I really would prefer to see an actual rationale for this change. So far you only assert that the current behavior is wrong somehow.

[ikedam]

I don't think this is a good way to resolve the problem pointed in JENKINS-30564. I prefer to add a checkbox "use this as the shortest timeout" to "Timeout minutes".

I think JENKINS-30564 is caused not for too few builds, but rather for that the project configuration is updated and the expected build duration gets longer. This can happen even when there're sufficient number of builds, and this request doesn't resolve those cases.

Though I don't think ElasticTimeOutStrategy is applicable to those cases anyway, allowing users configure fail-safe timeout duration is a better approach.

[ikedam]

How's work going?

[fbelzunc]

@ikedam I like your approach. I have just started to work on it.

[fbelzunc]

@ikedam @daniel-beck Would you mind to take a look at this?

[fbelzunc]

@ikedam What do you think now?

[fbelzunc]

@ikedam I think I did all in the way you said. Would you mind to review it again?

[ikedam]

Looks good. I want unit tests befere merging. Do you plan to add them? If not, I can do that instead.

[fbelzunc]

Let me give a try.

[fbelzunc]

@ikedam Tests added.

• Do you think they are ok?

[ikedam]

I also want following test cases:

• A case failSafe... is set to true and the default value is not used (the calcurated timeout is enough long).
• A case the default value is configured with variables.

The latter case may be a little hard to implement as variables are not used in existing tests. I suppose you can do that by adding ParametersAction to the build. See also StringParameterValue.

[ikedam]

I added some tests and created #49. Would you review that?

[fbelzunc]

Yes, I just did it.