-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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 non-root support in sysvinit script #4340
Conversation
What provides |
@andrewkroh Basically, if Filebeat plan to support some older distribution. CentOS5/Debian7/Ubuntu14.04. |
Thanks for the details on |
Can you please sign the CLA (contributor license agreement)? http://www.elasticsearch.org/contributor-agreement/ |
@andrewkroh |
@@ -42,8 +44,12 @@ if status | grep -q -- '-p' 2>/dev/null; then | |||
pidopts="-p $pidfile" | |||
fi | |||
|
|||
if [ -f /sbin/runuser ]; then |
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.
Rather than explicitly stating the path, I think it would be better to use the check described here. WDYT?
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.
Good point.
@@ -42,8 +44,12 @@ if status | grep -q -- '-p' 2>/dev/null; then | |||
pidopts="-p $pidfile" | |||
fi | |||
|
|||
if [ -f /sbin/runuser ]; then | |||
user_wrapper="runuser" |
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.
Do you also need to update to user_wrapperopts="-u $beat_user"
?
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.
It's same argument and order. So I can reuse user_wrapperopts
here.
@andrewkroh It seems like only a CLA need to be done from this checklist. But I had signed a CLA yesterday(with my github username and email address). Anything I can do? |
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. I'm going to wait to have @tsg review when he returns from vacation next week.
jenkins, test it |
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. Thanks for the contribution!
We should have a CHANGELOG entry on this one. @dreampuf could you add a line under the |
Jenkins test failure is not related. |
As #4333 mentioned, I found that the sysvinit script of beats will fail on non-root user launch the beats. Cause the arguments checker will be executed as root by default.
I add an optional
BEAT_USER
, which may come from/etc/default/$NAME
.