-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 file writable check in #configure #401
Conversation
👍 |
I will write a permission check for |
I'm not sure. I put some socket utilities in https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin/socket_util.rb. |
6f1cde3
to
22ca0b9
Compare
@repeatedly Could you review?
|
@repeatedly ping |
# @param [String] path File path | ||
# @return [Boolean] file is writable or not | ||
def writable?(path) | ||
if File.exists?(path) |
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.
Should use exist?
instead of exists?
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 do not mind. I will fix.
03d6829
to
9181ee8
Compare
@repeatedly fixed! |
# @param [String] path File path | ||
# @return [Boolean] file is writable or not | ||
def writable?(path) | ||
if File.exist?(path) |
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.
How about following code?
path = File.dirname(path) unless File.exist?(path) # or path = File.exist?(path) ? path : File.dirname(path)
File.writable?(path)
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. Fixed
9181ee8
to
fd1b901
Compare
add file writable check in #configure
Changes Unknown when pulling fd1b901 on writable_check into * on master*. |
--config-test
is very useful, but it does not check file permission. This proposition is to make a rule to check file permissions in #configure method of each plugin because I thought it is impossible to check file permissions for each configuration parameter of each plugin from Fluentd side (Fluentd can not know what each plugin wants to do)This pull request is just the first example of the proposition.
@repeatedly What do you think? Yes, each plugin author should support file permission check in #configure method of each plugin when we decide as this is an official way.
PS. @repeatedly Could you tell me if you have a right place to put the file permission check utility function?