-
Notifications
You must be signed in to change notification settings - Fork 512
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
Do not support SIGUSR1 and SIGUSR2 syscall handling in windows #970
Changes from all commits
eb87d5c
39b42d9
e4f5500
89acf3b
eb802aa
b91f182
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// +build !windows | ||
|
||
package notary | ||
|
||
import ( | ||
"os" | ||
"syscall" | ||
) | ||
|
||
// NotarySupportedSignals contains the signals we would like to capture: | ||
// - SIGUSR1, indicates a increment of the log level. | ||
// - SIGUSR2, indicates a decrement of the log level. | ||
var NotarySupportedSignals = []os.Signal{ | ||
syscall.SIGUSR1, | ||
syscall.SIGUSR2, | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
// +build windows | ||
|
||
package notary | ||
|
||
import "os" | ||
|
||
// NotarySupportedSignals does not contain any signals, because SIGUSR1/2 are not supported on windows | ||
var NotarySupportedSignals = []os.Signal{} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,8 @@ package utils | |
import ( | ||
"crypto/tls" | ||
"fmt" | ||
"os" | ||
"os/signal" | ||
"path/filepath" | ||
"strings" | ||
|
||
|
@@ -244,3 +246,20 @@ func AdjustLogLevel(increment bool) error { | |
logrus.SetLevel(lvl) | ||
return nil | ||
} | ||
|
||
// SetupSignalTrap is a utility to trap supported signals hand handle them (currently by increasing logging) | ||
func SetupSignalTrap(handler func(os.Signal)) chan os.Signal { | ||
if len(notary.NotarySupportedSignals) == 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. non-blocking-nit: can we test this case with some mocking? I'm fine with assigning this const to empty for the test and deferring to return it back to its original value, to mock what Windows would do There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I was actually going to see if I could try to set up CI for windows (not sure if we want to do it in this PR) so that we can get coverage numbers for it, in which case this should be tested. I'll follow up on this later today and if it proves to be a ton of work I'll stub out the const instead. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I think we can probably set this up. In case it goes horribly badly and doesn't work, I've added a mocking test for this in a commit we can revert once we have the CI set up. |
||
return nil | ||
|
||
} | ||
c := make(chan os.Signal, 1) | ||
signal.Notify(c, notary.NotarySupportedSignals...) | ||
go func() { | ||
for { | ||
handler(<-c) | ||
} | ||
}() | ||
|
||
return c | ||
} |
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.
Fine with leaving it in for explicit-ness but for my own understanding, we don't technically need it when using the
*_windows.go
filename right?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.
yep, not needed. I just like having the labels since we also have a
nowindows
that matches against everything else.