Add fluent-plugin-generate command #1427

okkez · 2017-01-20T08:10:21Z

okkez · 2017-01-20T08:11:37Z

This is workin progress.
I will add one more feature #1376 (comment)

okkez · 2017-01-25T05:51:00Z

Done my work.
Could you check this PR @tagomoris @repeatedly ?

tagomoris

I have 2 general points.

Can you move templates from lib/fluent/command/templates to templates? It's bad practice to have non-ruby script files under lib.

Could you add some tests for dumped configuration definitions? Especially for markdown and text. I want to see the actual output example of that feature.

tagomoris · 2017-01-26T09:06:17Z

lib/fluent/command/plugin_config_formatter.rb

+      end
+      dumped_config[plugin_class.name] = plugin_class.dump
+    end
+    puts __send__("dump_#{@format}", dumped_config)


It's better to call methods via case when clause. Using __send__("dump_#{@format}") makes code unreadable and hard to know the exact list of supported formats.

tagomoris · 2017-01-26T09:11:24Z

lib/fluent/configurable.rb

@@ -166,8 +166,8 @@ def merged_configure_proxy
        configurables.map{ |a| a.configure_proxy(a.name || a.object_id.to_s) }.reduce(:merge)
      end

-      def dump(level = 0)
-        configure_proxy_map[self.to_s].dump(level)
+      def dump


Could you change the method name from dump to dump_config_definition or something like that?
PluginClass.dump looks awful for that purpose.

tagomoris · 2017-01-26T09:44:43Z

lib/fluent/plugin/base.rb

@@ -28,6 +28,17 @@ class Base

      attr_accessor :under_plugin_development

+      def self.plugin_helpers


There are no need to show all plugin helpers enabled in plugins. For example, all input plugins enable event_emitter plugin helper, but it makes no sense for users. There are same things about thread, event_loop and some others.

So, how about to do this?:

store the list of plugin helpers enabled by helpers into class variable

return the list of plugin helpers enabled by helpers (concatenated list of ancestors)

add another method Fluent::PluginHelper.helpers_internal to bypass storing helper names for internal use about event_emitter, thread, retry_state and some others in plugin base classes.

module Fluent module PluginHelper module Mixin # ... end def helpers_internal(*snake_case_symbols) # it's actually rename of current #helpers # ... end def helpers(*sname_case_symbols) @_plugin_helpers_list ||= [] @_plugin_helpers_list << sname_case_symbols helpers_internal(*sname_case_symbols) end end end

Sounds nice. I will change implementation.

tagomoris · 2017-01-26T09:48:39Z

lib/fluent/command/plugin_config_formatter.rb

+  def dump_txt(dumped_config)
+    dumped = ""
+    plugin_helpers = dumped_config.delete(:plugin_helpers)
+    dumped << "helpers: #{plugin_helpers.join(',')}\n" if plugin_helpers


PluginClass.plugin_helpers always returns an array object, which may be empty for plugin classes which doesn't use any plugin helpers.
So this line should be unless plugin_helpers.empty?.

tagomoris · 2017-01-26T10:01:54Z

lib/fluent/command/plugin_config_formatter.rb

+    dumped_config[:plugin_helpers] = @plugin.class.plugin_helpers
+    @plugin.class.ancestors.reverse_each do |plugin_class|
+      next unless plugin_class.respond_to?(:dump)
+      next if plugin_class == Fluent::Plugin::Base


Why is Fluent::Plugin::Base removed? There seems no actual benefits to do it.

Because Fluent::Plugin::Base has no config_param and config_section.
Fluent::Plugin::Base.dump_config_definition always returns empty hash.
I think it is noise for us.

How about to omit classes which returns empty hash, instead of using class name?
There are some other classes which doesn't have any config_param and config_section.

Sounds good. Thanks!

tagomoris · 2017-01-26T10:10:25Z

lib/fluent/command/plugin_generator.rb

+
+class FluentPluginGenerator
+  attr_reader :type, :name
+  attr_reader :license


It's ok to fix the license as Apache-2.0.
If this feature supports GPLv2 or GPLv3 or any others, it means that Fluentd source code tree have the license files/templates of these licenses, or code to download these license files. It sounds very bad situation.

tagomoris · 2017-01-26T10:14:28Z

lib/fluent/command/plugin_generator.rb

+  end
+
+  def copy_license
+    # in gem_name directory


This method should download the original license from https://www.apache.org/licenses/LICENSE-2.0.txt and use it.
We can't assure the correctness of license text (Please remove the license file from template tree).

tagomoris · 2017-01-26T10:18:56Z

lib/fluent/command/plugin_generator.rb

+  def prepare_parser
+    @parser = OptionParser.new
+    @parser.banner = <<BANNER
+Usage: fluent-plugin-generate [options] <type> <name>


It's better to show the list of available types.

tagomoris · 2017-01-26T10:24:59Z

lib/fluent/command/templates/new_gem/test/plugin/test_filter.rb.erb

+    Fluent::Test::Driver::Filter.new(Fluent::Plugin::<%= class_name %>).configure(conf)
+  end
+
+  def test_failure


Use test 'failure' type testcases to introduce that feature to plugin authors.

tagomoris · 2017-01-26T10:27:23Z

test/helper.rb

@@ -146,6 +146,15 @@ def ipv6_enabled?
  end
 end

+def capture_stdout


Move this method to fluent/test/helpers because that file already have #capture_log method.

tagomoris · 2017-01-26T10:32:07Z

@okkez Nice implementation! I added general review comment and comments for some points.
@repeatedly please add some other points if you have.

repeatedly · 2017-01-26T11:56:15Z

lib/fluent/command/plugin_config_formatter.rb

+    init_engine
+    @plugin = Fluent::Plugin.__send__("new_#{@plugin_type}", @plugin_name)
+    dumped_config = {}
+    dumped_config[:plugin_helpers] = @plugin.class.plugin_helpers


These two lines are merged into dumped_config = {plugin_helpers: @plugin.class.plugin_helpers}

repeatedly · 2017-01-26T12:08:04Z

lib/fluent/command/plugin_config_formatter.rb

+      else
+        dumped << "\n"
+      end
+      dumped << "#{dump_section_txt(sub_section, level + 1)}"


Why need "#{}" ? Only dump_section_txt(sub_section, level + 1) has a problem?

repeatedly · 2017-01-26T12:16:42Z

lib/fluent/command/templates/new_gem/fluent-plugin.gemspec.erb

+
+  spec.add_development_dependency "bundler"
+  spec.add_development_dependency "rake"
+  spec.add_development_dependency "test-unit"


should add version constraint like ~> 3.0?

repeatedly · 2017-01-26T12:17:04Z

lib/fluent/command/templates/new_gem/README.md.erb

+
+TODO: write description for you plugin.
+
+## Configuration


Add ## installation section.

In this version we can do following: ```ruby class TestFoo < Test::Unit::TestCase CONF = config_element("ROOT", "", { "key" => "value" }) end ```

Because owned plugins does not includes Fluetn::PluginHelper::Mixin.

But these variables used in templates/plugin_config_formatter/section.md.erb lib/fluent/command/plugin_config_formatter.rb:167: warning: assigned but unused variable - required lib/fluent/command/plugin_config_formatter.rb:168: warning: assigned but unused variable - multi lib/fluent/command/plugin_config_formatter.rb:169: warning: assigned but unused variable - alias_name

okkez · 2017-01-30T07:52:57Z

@tagomoris @repeatedly I've pushed changes for all comments.
Could you check again?

repeatedly · 2017-02-09T19:44:16Z

LGTM but all tests failed.
Could you check test?

Because `exit(false)` raises SystemExit.

okkez · 2017-02-10T05:00:42Z

@repeatedly @tagomoris Passed all tests.

repeatedly · 2017-02-16T12:38:06Z

Thanks for your work!

tagomoris added feature request *Deprecated Label* Use enhancement label in general v0.14 work-in-progress labels Jan 20, 2017

okkez force-pushed the plugin-generator branch from 4612ce5 to 8e151ec Compare January 24, 2017 01:04

okkez changed the title ~~WIP: Add fluent-plugin-generate command~~ Add fluent-plugin-generate command Jan 25, 2017

tagomoris removed the work-in-progress label Jan 26, 2017

tagomoris requested changes Jan 26, 2017

View reviewed changes

repeatedly reviewed Jan 26, 2017

View reviewed changes

okkez added 19 commits January 30, 2017 13:21

Add fluent-plugin-generate

da3664e

Copy license first

11526c7

Display using license

52559a8

Be silent

27d2cd7

Add create_label and overwrite?

6b0abce

Overwrite generated files when destination exists

2cc7f7c

Fix a typo

8dcb7f9

Align

6d99ffd

Organize spec.files, spec.test_files

44d4a1b

Add missing spec.version

79a412c

Support all, quit and help in FluentPluginGenerator#overwrite?

2d5175f

Chmod 0755 bin/fluent-plugin-generate

62fadcd

Support --no-license option

2e723a7

Use #file instead of FileUtils#cp

fc26431

Parse @argv properly

dd2581f

Move capture_stdout to use other test

bbab5e4

Handle unknown license

031e171

Wait pid

d962784

Add test for fluent-plugin-generate

3300253

okkez and others added 16 commits January 30, 2017 13:21

Add installation section

2ffd60b

Enhance fluent-plugin-generate help

e87628a

Enhance fluent-plugin-config-format help

d1a54d4

Add -I option to fluent-plugin-config-format

33c6f0a

Move capture_stdout under Fluetn::Test::Helpers

caaf557

Use test "name" do ... style

56d67df

Extend Test::Unit::TestCase to use config_element

a5fe470

In this version we can do following: ```ruby class TestFoo < Test::Unit::TestCase CONF = config_element("ROOT", "", { "key" => "value" }) end ```

Specify version spec to generated gemspec

75dd130

Define Fluent::Test::Helpers#capture_stdout properly

43af372

Fix typos

3e7ec76

Stop displaying plugin helpers when owned plugins

ea79554

Because owned plugins does not includes Fluetn::PluginHelper::Mixin.

Dump nested section properly

2b60e8d

Simplify markdown output

0bf8c2c

Add test for fluent-plugin-config-format

2dac957

Initialize @_plugin_helpers_list to suppress warnings

0043507

okkez force-pushed the plugin-generator branch from 4c2af88 to 78e987c Compare January 30, 2017 04:23

okkez added 4 commits January 30, 2017 13:24

Be sure to initialize @_plugin_helpers_list

261796c

Define NoLicense#preamble same as ApacheLicense#preamble

8e4b749

Stop generating LICENSE file when no license

654a5a5

Assert timekey_zone dynamically

67d73e0

Rescue SystemExit by assert_raise

ecf5fa5

Because `exit(false)` raises SystemExit.

tagomoris approved these changes Feb 14, 2017

View reviewed changes

repeatedly merged commit f1e8fa4 into fluent:master Feb 16, 2017

repeatedly mentioned this pull request Feb 16, 2017

Plugin Generator #1376

Closed

okkez deleted the plugin-generator branch February 21, 2018 00:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add fluent-plugin-generate command #1427

Add fluent-plugin-generate command #1427

okkez commented Jan 20, 2017

okkez commented Jan 20, 2017

okkez commented Jan 25, 2017

tagomoris left a comment

tagomoris Jan 26, 2017

tagomoris Jan 26, 2017

tagomoris Jan 26, 2017

okkez Jan 27, 2017

tagomoris Jan 26, 2017

tagomoris Jan 26, 2017

okkez Jan 27, 2017

tagomoris Jan 27, 2017

okkez Jan 27, 2017

tagomoris Jan 26, 2017

tagomoris Jan 26, 2017

tagomoris Jan 26, 2017

tagomoris Jan 26, 2017

tagomoris Jan 26, 2017

tagomoris commented Jan 26, 2017

repeatedly Jan 26, 2017

repeatedly Jan 26, 2017

repeatedly Jan 26, 2017

repeatedly Jan 26, 2017

okkez commented Jan 30, 2017

repeatedly commented Feb 9, 2017

okkez commented Feb 10, 2017

repeatedly commented Feb 16, 2017

		@@ -28,6 +28,17 @@ class Base

		attr_accessor :under_plugin_development

		def self.plugin_helpers

Add fluent-plugin-generate command #1427

Add fluent-plugin-generate command #1427

Conversation

okkez commented Jan 20, 2017

okkez commented Jan 20, 2017

okkez commented Jan 25, 2017

tagomoris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tagomoris commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

okkez commented Jan 30, 2017

repeatedly commented Feb 9, 2017

okkez commented Feb 10, 2017

repeatedly commented Feb 16, 2017