Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
hooks: default timeout #3282
Conversation
All checks have failed
|
@stolowski Why do we have to increase the timeout to 10min? |
|
I need to point out that the prepare-device hook has been known to take 10-15 minutes on some devices. It's been reduced thanks to tvoss's work, but might still be something like that in the future. I'd suggest leaving this default as is, but also adding a 30 minute timeout to the |
Some checks were not successful
|
@chipaca thanks for pointing out, done. |
|
@chipaca @stolowski the key generation was taking 10+ mins, but that's not a hook, the prepare-device hook doesn't do key generation, it sets some config options usually, it shouldn't take forever! |
|
Ah! i stand corrected then :-) |
stolowski
added some commits
May 9, 2017
Some checks were not successful
Some checks were not successful
| @@ -37,7 +37,7 @@ func init() { | ||
| } | ||
| func configureHookTimeout() time.Duration { | ||
| - timeout := 5 * time.Minute | ||
| + timeout := 10 * time.Minute |
pedronis
May 9, 2017
Contributor
it's not clear from the forum discussion if we want this too? @niemeyer?
| @@ -280,6 +280,7 @@ func snapCmd() string { | ||
| var syscallKill = syscall.Kill | ||
| var cmdWaitTimeout = 5 * time.Second | ||
| +var defaultHookTimeout = 10 * time.Minute |
| + | ||
| + s.manager.Ensure() | ||
| + completed := make(chan struct{}) | ||
| + go func() { |
pedronis
May 9, 2017
Contributor
I see the other tests are like this, but naively I'm not quite sure why need the goroutine here
stolowski
May 9, 2017
Contributor
You're right, these tests can be simplified. Updated my test case. If we are OK with this change I will simplify existing tests in a separate PR.
All checks have passed
zyga
requested a review
from
niemeyer
May 10, 2017
All checks have passed
|
Restored config hook timeout back to 5 minutes for now so that the change can land and config hook tweaking can be done in a separate PR. |
stolowski commentedMay 9, 2017
Set default timeout for hooks to 10 min, if not provided by hook setup.
Also increased the timeout of configure hook to 10 min; we could remove a specific configure hook timeout and use a default, but I think it's good to keep it separate in the code for fine-tuning (also, configure hook timeout can be tweaked via env var).