-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix jib errors on ctrl-c #1248
Fix jib errors on ctrl-c #1248
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1248 +/- ##
=======================================
Coverage 42.18% 42.18%
=======================================
Files 96 96
Lines 4238 4238
=======================================
Hits 1788 1788
- Misses 2276 2277 +1
+ Partials 174 173 -1
Continue to review full report at Codecov.
|
pkg/skaffold/util/process.go
Outdated
@@ -26,13 +26,14 @@ import ( | |||
// IsTerminatedError returns true if the error is type exec.ExitError and the corresponding process was terminated by SIGTERM | |||
// This error is given when a exec.Command is ran and terminated with a SIGTERM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to fix up the comments.
pkg/skaffold/util/process.go
Outdated
case *exec.ExitError: | ||
status := exitErr.Sys().(syscall.WaitStatus) | ||
|
||
return status.Signaled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think SIGSEGV, SIGBUS, etc. should be masked.
@briandealwis This should be better now. I realized we just have to test for the context's error and see if it was cancelled. |
@@ -465,6 +465,12 @@ func DependenciesForArtifact(ctx context.Context, a *latest.Artifact) ([]string, | |||
} | |||
|
|||
if err != nil { | |||
// if the context was cancelled act as if all is well | |||
// TODO(dgageot): this should be even higher in the call chain. | |||
if ctx.Err() == context.Canceled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will mask all errors. Perhaps you could log the error at a DEBUG level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, the idea is to not show annoying errors when the user presses ctrl-c. No matter what the error is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I suggested DEBUG :-) Just in case those errors are due to something other than SIGTERM/KILL. Anyways, it's not a blocked from me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to add debug logs
Signed-off-by: David Gageot <david@gageot.net>
Signed-off-by: David Gageot <david@gageot.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #1247