Skip to content
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

Raise minimum required ruby version #4288

Merged
merged 1 commit into from
Sep 8, 2023

Conversation

kenhys
Copy link
Contributor

@kenhys kenhys commented Sep 5, 2023

Which issue(s) this PR fixes:

N/A

What this PR does / why we need it:

The service discovery plugin helper use Array#prepend, it means that the ruby version must be 2.5 or later precisely.

fluentd.work/lib/fluent/plugin_helper/service_discovery.rb:71:in
service_discovery_configure': undefined method prepend' for
[]:Array (NoMethodError) from
/work/fluent/fluentd/fluentd.work/lib/fluent/plugin/out_

Docs Changes:

fluent/fluentd-docs-gitbook#471

Release Note:

N/A

Copy link
Contributor

@cosmo0920 cosmo0920 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! 👍

@daipom
Copy link
Contributor

daipom commented Sep 6, 2023

Thanks!
Shouldn't we update the minimum version to 2.7 since the minimum version tested in CI is 2.7?

@kenhys
Copy link
Contributor Author

kenhys commented Sep 6, 2023

It depends on perspective:

  • Precisely set minimum:
    • Pros: can run a bit older even though already reached EOL.
    • Cons: allow non-supported version to run with.
  • Only set CI certified version:
    • Pros: can recommend verified version with CI.
    • Cons: cut off ruby version actually work.

Created PR based on the former, but reconsidered it should be the latter.

Thanksm, I'll fix it.

The service discovery plugin helper use Array#prepend, it means that
the ruby version must be 2.5 or later precisely.

   fluentd.work/lib/fluent/plugin_helper/service_discovery.rb:71:in
   `service_discovery_configure': undefined method `prepend' for
   []:Array (NoMethodError) from
   /work/fluent/fluentd/fluentd.work/lib/fluent/plugin/out_

NOTE: It works with Ruby 2.5 or later, but 2.x had already
reached EOL. At least, it may better to set verified version with CI.

Signed-off-by: Kentaro Hayashi <[email protected]>
@kenhys
Copy link
Contributor Author

kenhys commented Sep 6, 2023

Fixed.

Copy link
Contributor

@daipom daipom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends on perspective:

* Precisely set minimum:
  
  * Pros: can run a bit older even though already reached EOL.
  * Cons: allow non-supported version to run with.

* Only set CI certified version:
  
  * Pros: can recommend verified version with CI.
  * Cons: cut off ruby version actually work.

Created PR based on the former, but reconsidered it should be the latter.

Thanksm, I'll fix it.

I see!
At least, we need this fix.

We can think about whether we should update the version to 2.7 or not later.

@daipom
Copy link
Contributor

daipom commented Sep 6, 2023

Fixed.

Thanks!

@kenhys
Copy link
Contributor Author

kenhys commented Sep 6, 2023

We can think about whether we should update the version to 2.7 or not later.

When td-agent reached EOL (Dec, 2023), we will drop 2.7 from CI.

@daipom daipom merged commit eb30a6e into fluent:master Sep 8, 2023
13 of 16 checks passed
@daipom daipom modified the milestone: v1.16.3 Oct 10, 2023
@daipom daipom added this to the v1.17.0 milestone Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants