Configurable default HealthCheck timeout#121
Conversation
plamen-bardarov
changed the title
cf-foundation-community-automation
bot
moved this to Inbox
in Application Runtime Platform Working Group
depot/transformer/transformer.go
Outdated
| if timeout <= 0 { | ||
| timeout = int(t.declarativeHealthCheckDefaultTimeout / time.Millisecond) | ||
| } | ||
| if timeout <= 0 { |
There was a problem hiding this comment.
Can you add a test for this case where the declarativeHealthCheckDefaultTimeout is set to a negative number?
There was a problem hiding this comment.
Also maybe we should log in this case? Something like: you gave us an invalid value for X we are setting it to Y instead.
There was a problem hiding this comment.
I think its better if we only accept valid values for this field instead of relying on this default if the job is misconfigured so I added it to the validation method. However since we receive an already validated object from the rep, testing this is not possible in the executor 😕. I left the <=0 check just in case something changes.
| } | ||
|
|
||
| func (t *transformer) applyCheckDefaults(timeout int, interval time.Duration, path string) (int, time.Duration, string) { | ||
| if timeout == 0 { |
There was a problem hiding this comment.
Can you move this logic about defaults to the initializer package where the config is read in? That way the t.declarativeHealthCheckDefaultTimeout property is always correct.
There was a problem hiding this comment.
Moved it to the configuration option, which is still called only during initialization.
add tests for negative or zero values for `enable_declarative_healthcheck` move default timeout setting to initialization
github-project-automation
bot
moved this from Inbox
to Pending Merge | Prioritized
in Application Runtime Platform Working Group
github-project-automation
bot
moved this from Pending Merge | Prioritized
to Done
in Application Runtime Platform Working Group
2 participants
Summary
Make the declarative Liveness Healthcheck default timeout configurable.
Backward Compatibility
Breaking Change? No