snap: abort install with ctrl+c#2401
Conversation
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Thanks for this change!
One trivial, and one detail we need to discuss.
| if err != nil { | ||
| fmt.Fprintf(Stderr, err.Error()+"\n") | ||
| } | ||
| os.Exit(1) |
There was a problem hiding this comment.
That doesn't look right.. we need to ask for a stop rather than killing whatever is in progress midway through. Let's catch up online to discuss.
|
|
||
| pb := progress.NewTextProgress() | ||
| if err = sto.Download(name, targetFn, &snap.DownloadInfo, pb, opts.User); err != nil { | ||
| if err = sto.Download(nil, name, targetFn, &snap.DownloadInfo, pb, opts.User); err != nil { |
There was a problem hiding this comment.
Instead of passing nil, I suggest using context.TODO() everywhere we lack such a value, so that we know a context parameter is always supposed to exist. Otherwise we'll have to do if ctx != nil && everywhere, which is boring, and hopefully unnecessary since we'll gradually add it in the places we intend to support context.
| logger.Debugf("Retyring %s, attempt %d, delta time=%v ms", reqOptions.URL, attempt.Count(), delta) | ||
| } | ||
| resp, err = s.doRequest(client, reqOptions, user) | ||
| if ctx != nil && cancelled(ctx) { |
There was a problem hiding this comment.
With logic suggested above, we can drop the != nil.
mvo5
left a comment
mvo5
left a comment
There was a problem hiding this comment.
Looks good, one small question.
|
|
||
| func setupAbortHandler(changeId string) { | ||
| // Intercept sigint | ||
| c := make(chan os.Signal, 2) |
There was a problem hiding this comment.
Pardon my silly question, but why a capacity of 2 (and not e.g. 1 or 3)?
There was a problem hiding this comment.
No particular reason... it was just the value I saw in some example go code...
3 participants
Abort snap install via ctrl+c. Includes update to the latest tomb package.
LP: #1592074