jgraichen / telegraf-ruby

Send events from Ruby to a Telegraf agent
GNU Lesser General Public License v3.0
13 stars 7 forks source link

Fix Ruby 3 keyword arg deprecation #14

Closed machty closed 3 years ago

machty commented 3 years ago

Fixes Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call deprecation warnings in 2.7.

Since we can't change how Sidekiq passes the middleware args back to the initializer, I think downgrading to options/fetch is the only option.

machty commented 3 years ago

Pushed a fix, and actually ran the specs this time (I was expecting Travis or some CI to be connected)

Note that this spec is failing, not sure if related

  1) Telegraf with UNIXGRAM socket write points
     Failure/Error: socket.accept_nonblock.read_nonblock(4096)

     Errno::EOPNOTSUPP:
       Operation not supported on socket - accept(2)
     # ./spec/spec_helper.rb:39:in `socket_read'
     # ./spec/telegraf_spec.rb:43:in `block (3 levels) in <top (required)>'
     # ./spec/spec_helper.rb:31:in `block in <module:Socket>'
     # ./spec/spec_helper.rb:15:in `block (2 levels) in <module:Tmpdir>'
     # ./spec/spec_helper.rb:13:in `block in <module:Tmpdir>'
jgraichen commented 3 years ago

(I was expecting Travis or some CI to be connected)

Travis CI is dead but we have Github Actions. I probably just configured it wrongly for (external) merge requests. I did push a possible fix, if you rebase this MR it might just run the specs.

jgraichen commented 3 years ago

Since we can't change how Sidekiq passes the middleware args back to the initializer

It does look possible to be changed to handle keyword arguments as keyword arguments for each middleware entry but I am fine with changing it here too.

machty commented 3 years ago

@jgraichen I tried your suggested commit but without digging super deep into it, I don't think we can make it work with Sidekiq's middleware API; specifically, the chain.add seems to only expect kwargs and not a positional param like agent (and I haven't found examples in the wild that use positional args and the docs just use kwargs).

We're using telegrafy-ruby and this is blocking us from upgrading Ruby 3; would you be willing to merge the PR as is and maybe revisit this later?

machty commented 3 years ago

Any update on this? We're using a fork that fixes the issue until this can be merged.

jgraichen commented 3 years ago

specifically, the chain.add seems to only expect kwargs and not a positional param like agent (and I haven't found examples in the wild that use positional args and the docs just use kwargs).

Yes, sidekiq would need to accept, store, and forward keyword arguments for middlewares. Otherwise, no middleware will be able to use keyword arguments at all in Ruby 3.

jgraichen commented 3 years ago

I added Ruby 3.0 to CI and changed the sidekiq middleware to not use keyword arguments at all. This has been released as v2.0.0.