-
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
fix to generate storage local path using usage #1438
Conversation
@repeatedly could you review this change? |
@@ -64,7 +64,8 @@ def configure(conf) | |||
end | |||
end | |||
elsif root_dir = owner.plugin_root_dir | |||
@path = File.join(root_dir, 'storage.json') | |||
basename = (conf.arg && !conf.arg.empty?) ? "storage.#{conf.arg}.json" : "storage.json" |
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.
conf.arg
can contain space like <storage foo bar>
.
So need to validate or convert unexpected charactors.
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.
Okay, I'll add validation for usage.
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 conf.arg
will be given by :usage in storage_create?
Or users should specify <storage args>
by themselves?
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.
Ah, I got it.
We should fill up usage
when conf is Fluent::Config::Element
's instance case: https://github.com/fluent/fluentd/blob/master/lib/fluent/plugin_helper/storage.rb#L56
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 behavior is a limitations for now.
One comment. Others are good. |
Pushed a commit to add validation for usage. |
This feature is very useful for plugins which uses 2 or more storage instances, with
root_dir
configuration.