WIP: Follow up to https://github.com/openshift/router/pull/483#515
WIP: Follow up to https://github.com/openshift/router/pull/483#515frobware wants to merge 8 commits intoopenshift:masterfrom
Conversation
* a value larger than time.ParseDuration can handle is clipped to the HAProxy max timeout * a value that cannot be properly parsed for other reasons is set to empty instead of being silently allowed To check that time.ParseDuration is experiencing overflow, add a new ParseHAProxyDuration to the util package so we can evaluate the errors returned by time.ParseDuration without sacrificing its authority in parsing time strings.
Modify the function signature of ParseDuration to remove MaxTimeout checks. Update comments to reflect that the caller is now responsible for any MaxTimeout checks. Add unit tests to exercise the boundary conditions for int64 durations. Include test cases for small values, large values, and values that should cause an OverflowError. Correct erroneous/missing test inputs for microseconds.
Introduced `haproxyMaxTimeout` variable to store parsed max timeout for HAProxy. Added an `init()` function that parses `templateutil.HaproxyMaxTimeout` into `haproxyMaxTimeout` and panics on invalid duration. Modified `clipHAProxyTimeoutValue()` to use the newly introduced `haproxyMaxTimeout` for clipping values that exceed the HAProxy maximum. Note: This commit follows a semantic change to `haproxytime.ParseDuration` (previous commit), which moved the responsibility for max timeout handling to the caller.
This is largely what I had some commits ago; I experimented with some changes that required the unit to be discrete in the test case but I since abandoned that. Encoding expectedDuration arguably makes the failure cases more obvious as the expectedDuration is simply zero without needing to specify any units.
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
frobware
changed the title
|
/test unit |
|
@frobware: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
2 participants
See #483