Allow to modify the default timeout for tiller #1158
Conversation
pkg/proxy/proxy.go
Outdated
| if envTimeout := os.Getenv(envVar); envTimeout != "" { | ||
| timeout, err = strconv.Atoi(envTimeout) | ||
| if err != nil { | ||
| log.Errorf("Unable to use custom timeout. Failed to parse %s as int: %v", envVar, err) |
There was a problem hiding this comment.
Avoid failures on purpose, if the timeout is not properly set, fallback to the default.
There was a problem hiding this comment.
I'd personally rather know that the config value that I have explicitly set was incorrect, rather than silently not knowing? Why hide the error?
If it is because this function is only called run-time and so would stop the service, we could instead create an init function (https://golang.org/doc/effective_go.html#init ) which verifies the values, but again, I don't think we should as these can be treated as command-line options (and we can fail there when parsing).
There was a problem hiding this comment.
I'm OK if you want to land this as is (though check through the thoughts below to see what you think), but more generally, I think these options should be command-line options which are checked in the main.go when parsing flags at startup, together with something like setFlagFromEnv (see it's use in helm). Having them as run-time checks blurs the interface for config (and as you found, can leave awkward situations like ignoring errors because we don't want the server to stop some time later after successfully starting.
So what I would have expected was support for an --operation-timeout flag which, if not set, checks the env var (during initialization, using setFlagFromEnv), which if not set, defaults to the current value. (or 4 separate options for install, rollback, upgrade, delete - though I'm not sure why we need these separate?)
Let me know what you think.
pkg/proxy/proxy.go
Outdated
| if envTimeout := os.Getenv(envVar); envTimeout != "" { | ||
| timeout, err = strconv.Atoi(envTimeout) | ||
| if err != nil { | ||
| log.Errorf("Unable to use custom timeout. Failed to parse %s as int: %v", envVar, err) |
There was a problem hiding this comment.
I'd personally rather know that the config value that I have explicitly set was incorrect, rather than silently not knowing? Why hide the error?
If it is because this function is only called run-time and so would stop the service, we could instead create an init function (https://golang.org/doc/effective_go.html#init ) which verifies the values, but again, I don't think we should as these can be treated as command-line options (and we can fail there when parsing).
pkg/proxy/proxy.go
Outdated
| timeout, err = strconv.Atoi(envTimeout) | ||
| if err != nil { | ||
| log.Errorf("Unable to use custom timeout. Failed to parse %s as int: %v", envVar, err) | ||
| timeout = defaultTimeoutSeconds |
There was a problem hiding this comment.
This is superfluous I think? (already set above on 249).
There was a problem hiding this comment.
if err is set, timeout is set back to 0, that's why I need to re-set it.
pkg/proxy/proxy.go
Outdated
| @@ -240,6 +243,21 @@ func unlock(name string) { | |||
| appMutex[name].Unlock() | |||
| } | |||
|
|
|||
| // Returns a timeout based on the value of a environment variable. | |||
| // defaults to 300s (5min) | |||
| func getTimeout(envVar string) int64 { | |||
There was a problem hiding this comment.
A thought on this function: providing a generic function means that it can be called with useless values (by mistake). Although we could improve this with something like:
type EnvVar string
const INSTALL_TIMEOUT_ENV_VAR EnvVar = "INSTALL_TIMEOUT"
...
then update the getTimeout signature to
func getTimeout(envVar EnvVar) int64 {
(for a similar example, see https://golang.org/doc/effective_go.html#constants )
I don't think we should, as these are really command-line options IMO. See my main comment.
|
Okay, thank you for the feedback @absoludity, it's true that using a flag can be more convenient, let me work on that. |
Fixes #1090
Add option to setup this through the chart.
I have tested this manually.
cc/ @agill17