Support limiting minimum instances to a time range #404
Conversation
erikhakansson
force-pushed
the
timeLimitOnMinInstances
branch
from
8ca8bbf
to
fae79b2
Compare
| @@ -130,6 +130,8 @@ | |||
|
|
|||
| private int minimumNumberOfInstances; | |||
|
|
|||
| private MinimumNumberOfInstancesTimeRangeConfig minimumNumberOfInstancesTimeRangeConfig; | |||
There was a problem hiding this comment.
What about old configs not having this property? Should you manage that in readResolve?
There was a problem hiding this comment.
IIUC There is a nullcheck for its usage
There was a problem hiding this comment.
Correct, there's a null check. And I did a quick check now and couldn't find anywhere that I had missed checking. :)
|
I won't have time to review this for a couple days. |
|
@jvz do you have the bandwidth to review this? |
| } | ||
|
|
||
| // For some reason, all days will validate against this method so no need to repeat for each day. | ||
| public FormValidation doCheckMonday(@QueryParameter boolean monday, |
There was a problem hiding this comment.
Maybe a different name if it's valid for all days?
There was a problem hiding this comment.
Ideally, yes, but it's some sort of Jelly magic based on the field names.
| import java.util.Map; | ||
|
|
||
| public class MinimumNumberOfInstancesTimeRangeConfig { | ||
|
|
There was a problem hiding this comment.
The field names are excessively long due to the class name prefix. Is this prefix required?
| </f:entry> | ||
| </f:rowSet> | ||
| </f:optionalBlock> | ||
| <f:entry /> |
There was a problem hiding this comment.
Not required, but it gets really cramped and ugly without it.
|
Merging tomorrow if no one has any objections |
Follow-up to PR #401, this PR adds the option to limit minimum number of instances to a specific time range.
I got some wishes for that from work so that we don't have build agents up and running all weekend when no one is working.