Using env variable to set vmware timeout value #1078
Conversation
pkg/blockstorage/vmware/vmware.go
Outdated
| if err == nil { | ||
| return iv | ||
| } |
There was a problem hiding this comment.
Suggest:
if err == nil || iv > 0 {
return iv
}
To avoid invalid values.
| os.Setenv(vmWareTimeoutMinEnv, "7") | ||
| timeout = time.Duration(getEnvAsIntOrDefault(vmWareTimeoutMinEnv, int(defaultWaitTime/time.Minute))) * time.Minute | ||
| c.Assert(timeout, Equals, 7*time.Minute) | ||
|
|
There was a problem hiding this comment.
Also test for "0" or "-1". What values would you expect?
|
|
||
| func getEnvAsIntOrDefault(envKey string, def int) int { | ||
| if v, ok := os.LookupEnv(envKey); ok { | ||
| iv, err := strconv.Atoi(v) |
There was a problem hiding this comment.
Log error. Defaulting on hidden failures are difficult to diagnose. It will help if you log a warning, or similar, to stderr
Another option - log Info to stdout whenever you default.
Message must include the environment variable and the value you are defaulting to.
If checking error, then return non-default value whenever err == nil, otherwise you are ignoring legitimate integer values - this all relates to my first point: someone set a legitimate integer value, but its silently being ignored when supplied, why?
While the code will work for the application, it seems as though the current implementation could cause frustration in operation.
There was a problem hiding this comment.
@aaron-kasten logging the case where an invalid option was provided
| @@ -350,3 +358,14 @@ func (p *FcdProvider) VolumesList(ctx context.Context, tags map[string]string, z | |||
| func (p *FcdProvider) SnapshotsList(ctx context.Context, tags map[string]string) ([]*blockstorage.Snapshot, error) { | |||
| return nil, errors.New("Not implemented") | |||
| } | |||
|
|
|||
| func getEnvAsIntOrDefault(envKey string, def int) int { | |||
| if v, ok := os.LookupEnv(envKey); ok { | |||
There was a problem hiding this comment.
nit: Personal bias toward less nesting. if ...; !ok { return def }, then return def again below if you have a reason to.
Change Overview
The following PR adds a vmWare specific environment variable to set the timeout of vmware calls.
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
Will manual test as well.