timeoutSeconds config in .tbrc.yml #327
Conversation
config/config.go
Outdated
| @@ -151,6 +152,11 @@ func Init(ctx context.Context, config Config, opts InitOptions) (*engine.Engine, | |||
| return nil, errors.New(errkind.Invalid, "no registries defined", op) | |||
| } | |||
|
|
|||
|
|
|||
| if config.TimeoutSeconds != nil && *config.TimeoutSeconds < 5 || *config.TimeoutSeconds > 3600 { | |||
There was a problem hiding this comment.
Does this need to be a pointer? Why not just treat the zero value (literally 0 in this case lol) as meaning no timeout? Based on this validation 0 isn't even allowed as a valid value so why not use it for this?
I think this make make things simpler and this is pretty standard practice in Go. For example, see the Timeout field on http.Client: https://pkg.go.dev/net/http#Client
A Timeout of zero means no timeout.
There was a problem hiding this comment.
that's a good point, though is there a way to know if someone explicitly set timeoutSeconds: 0 or if it's just not provided in .tbrc.yml? i'm asking because we're defaulting to 3600 when the value is not provided and i'm not sure if there is a way to distinguish between it not being set or it being set to 0
There was a problem hiding this comment.
There isn't any way to distinguish, someone could put timeoutSeconds: 0 in their .tbrc.yml but I guess the question is does it matter? I don't see why anyone would do that, and even if they did I don't think it matters, the behaviour should be the same
config/config.go
Outdated
| @@ -45,6 +45,7 @@ type Config struct { | |||
| Playlists map[string]playlist.Playlist `yaml:"playlists"` | |||
| Overrides map[string]service.ServiceOverride `yaml:"overrides"` | |||
| Registries []registry.Registry `yaml:"registries"` | |||
| TimeoutSeconds *int `yaml:"timeoutSeconds"` | |||
There was a problem hiding this comment.
Nice job having Seconds in the name to make it unambiguous
engine/engine.go
Outdated
| @@ -71,6 +72,9 @@ type Options struct { | |||
| GitClient git.Git | |||
| // DockerOptions is used to customize docker operations. | |||
| DockerOptions docker.Options | |||
| // TimeoutSeconds is a limit to how long a docker image pull will last | |||
| // If no value is provided, it defaults to 3600 | |||
| TimeoutSeconds int | |||
There was a problem hiding this comment.
Why not just make this a time.Duration? That's what it's meant to be might as well store it as that. Also means you don't need to convert it every time you use it. Also can rename it to just Timeout then. Same applies to Engine.timeoutSeconds
There was a problem hiding this comment.
sounds good, i used time.Duration all over but i needed to use int for the types used to parse the .tbrc.yml. i was getting an unmarshal error for some reason
There was a problem hiding this comment.
yeah it needs to be an int for the config, but for the Engine field it should be a time.Duration. Basically at the config layer we read it as an int but then convert it to a duration and use it as that in the rest of the app
engine/engine.go
Outdated
| // TimeoutSeconds is a limit to how long a docker image pull will last | ||
| // If no value is provided, it defaults to 3600 | ||
| TimeoutSeconds int |
There was a problem hiding this comment.
Is this only gonna be for docker pulls? Wouldn't we want this to be a general purpose timeout for all operations?
There was a problem hiding this comment.
i ended up using it for all operations as well
There was a problem hiding this comment.
Nice I think that makes the most sense, we should have all timeouts be configurable. We can always add more specific timeout overrides in the future if needed
l0rdcafe
changed the title
config/config.go
Outdated
| var timeout time.Duration | ||
| if config.TimeoutSeconds != 0 { | ||
| timeout = time.Duration(config.TimeoutSeconds) * time.Second | ||
| } else { | ||
| // default to 60 min timeout when not provided in .tbrc.yml | ||
| timeout = time.Duration(3600) * time.Second | ||
| } |
There was a problem hiding this comment.
Nit, could simplify this a bit by assigning the default along with the declaration and eliminate the else case.
There was a problem hiding this comment.
sounds good, thanks for the suggestion. looks simpler now
config/config.go
Outdated
| timeout = time.Duration(config.TimeoutSeconds) * time.Second | ||
| } else { | ||
| // default to 60 min timeout when not provided in .tbrc.yml | ||
| timeout = time.Duration(3600) * time.Second |
There was a problem hiding this comment.
Nit, you don't need to explicitly convert numeric constants, i.e. 3600 * time.Second works
config/config.go
Outdated
| @@ -158,12 +158,10 @@ func Init(ctx context.Context, config Config, opts InitOptions) (*engine.Engine, | |||
| return nil, errors.New(errkind.Invalid, fmt.Sprintf("Invalid timeoutSeconds value '%d' in .tbrc.yaml. Values must be between 5 and 3600 inclusive", config.TimeoutSeconds), op) | |||
| } | |||
|
|
|||
| var timeout time.Duration | |||
| // default to 60 min timeout when not provided in .tbrc.yml | |||
| timeout := 3600 * time.Second | |||
There was a problem hiding this comment.
Actually, you could have even done 60 * time.Minute 😄
There was a problem hiding this comment.
haha great call, let me do that instead 😄
No description provided.