(MODULES-7018) Fix Zero Timeout Behavior #225
Conversation
Some other Puppet Exec providers will treat a `timeout` value of `0` as an infinite timeout. We have chosen not to support an infinite timeout in this provider, so this change will ensure that a sensible value is used if the user passes in `0` as the timeout. It also fixes the README documentation to make it clearer that `0` is not supported, and that attempting to use it will result in the default timeout of 300 seconds being used instead.
RandomNoun7
force-pushed
the
tickets/master/MODULES-7018-Fix-timeout-zero-behavior
branch
from
6354421 to
ffc970d
Compare
RandomNoun7
requested review from
jpogran,
Iristyle,
glennsarti and
michaeltlombardi
RandomNoun7
changed the title
| @@ -177,7 +177,7 @@ Lists the expected return code(s). If the executed command returns something els | |||
|
|
|||
| ##### `timeout` | |||
|
|
|||
| Sets the maximum time in seconds that the command should take. Valid options: Number or string representation of a number. Default: 300. | |||
| Sets the maximum time in seconds that the command should take. Valid options: Number or string representation of a number. Default: 300. A value of `0` for this property will result in using the default timeout of 300. Inifinite timeout is not supported in this module, but large timeouts are allowed if needed. | |||
There was a problem hiding this comment.
Is there a maximum allowed timeout?
There was a problem hiding this comment.
The module code does not enforce one.
| # as an infinite timeout. We don't support infinite, so for the case | ||
| # of a user specifying zero, we sub in the default value of 300 | ||
| # seconds. | ||
| if (timeout_ms == 0) then timeout_ms = 300 * 1000 end |
There was a problem hiding this comment.
This seems really far down the callstack given we already perform a calculation for seconds to milliseconds in the provider code at
That would give us the opportunity to emit a warning to the user via Puppet.warning as well.
It actually seems best served to correct this value in the munge for the type, but since the type is defined elsewhere, we're not afforded that opportunity. Seems like provider is the next best spot.
Had you considered that approach instead?
There was a problem hiding this comment.
I considered higher in the stack, but decided to put it as close to the eventual preparation of the PowerShell code as possible. It’s a habit from my database days I guess where the general rule is that a constraint on the way data should be handled should happen as close to the storage and retrieval of the data as possible.
RandomNoun7
deleted the
tickets/master/MODULES-7018-Fix-timeout-zero-behavior
branch
Some other Puppet Exec providers will treat a
timeoutvalue of0asan infinite timeout. We have chosen not to support an infinite timeout
in this provider, so this change will ensure that a sensible value is
used if the user passes in
0as the timeout. It also fixes the READMEdocumentation to make it clearer that
0is not supported, and thatattempting to use it will result in the default timeout of 300 seconds
being used instead.