-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Add pid-file flag to agent #128
Conversation
pidFile, err := os.OpenFile(pidPath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0666) | ||
|
||
if err != nil { | ||
fmt.Errorf("Could not open pid file: %v", err) |
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.
These errors are never sent to stdout.
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, good point 😃 It seems like the rest of the functions in agent
return an error rather than writing directly to log. It also makes sense that the user would want the agent to error out rather than start up and not save its pid, causing them to possibly lose automatic control and require human intervention to fix.
I'll change it to return an error like everything else in agent.go
, unless it is preferred this fails softly?
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.
Yeah, I would prefer to fail hard.
stat, err := os.Stat(pidPath) | ||
|
||
if err != nil { | ||
if stat.IsDir() { |
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 crashes now. I noticed when you got the errors printed that OpenFile will throw this when trying to open a directory:
Could not open pid file: open testing: is a directory
So you would be covered even if you remove the os.Stat
check since we now fatal here.
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.
Gotchya, yea that's why I was confused =P I thought you meant we should check incase they changed the OpenFile
api. Ok, I'll just rebase out that commit so I'm not cluttering the log.
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.
Awesome, thanks! Yeah sorry, I was not clear.
LGTM! Thanks alot! |
Resolves #106 by adding a
-pid-file=<path>
flag to agent which allows it to store it's PID in a pid file. This is useful for sending an interrupt if not using a daemon manager or sending special signals the agent recognizes such asSIGHUP
to update check definitions.This PR also includes the addition of the flag in the website docs for agent.