Skip to content

Conversation

@hibariya
Copy link
Contributor

@hibariya hibariya commented Apr 11, 2025

  • Added an explicit require 'logger' to fix errors (like this) in some older versions of ActiveJob
  • Added a bit older Ruby and ActiveJob to the test matrix
  • Reduced the number of ruby x active_job combinations for too older versions

@hibariya hibariya force-pushed the update-ci branch 2 times, most recently from 9daa830 to 48aa3dc Compare April 11, 2025 01:12
@hibariya hibariya force-pushed the update-ci branch 2 times, most recently from d6e38a1 to 4b756a0 Compare April 11, 2025 01:26
@hibariya hibariya marked this pull request as ready for review April 11, 2025 01:27
@hibariya hibariya requested a review from koic April 11, 2025 01:28
@koic
Copy link
Member

koic commented Apr 11, 2025

I'm not sure from a review perspective, what version of Ruby is required? Since this affects users, it's different from just tweaking CI settings. It would be good to specify require_ruby_version in the gemspec.

@hibariya
Copy link
Contributor Author

@koic Good question! I think no one has decided which Ruby versions this gem supports. I don't foresee a major problem with leaving it unspecified for now, but happy to change this if someone needs it. In this PR, I limited the number of combinations of Ruby and ActiveJob in the CI just because some newer ActiveJob versions require newer Ruby.

@koic
Copy link
Member

koic commented Apr 11, 2025

I think the minimum Ruby version tested in CI matrix should match the required_ruby_version specified for installation. Otherwise, there could be unexpected issues between the CI environment and user runtime. It may be a good opportunity to consider aligning them now.

@hibariya
Copy link
Contributor Author

@koic Yeah, that makes sense to me too 👍 Added: 0539ea7

@koic
Copy link
Member

koic commented Apr 11, 2025

For example, since the current ruby:latest is Ruby 3.5, it is unclear whether the CI matrix for ruby:3.4 is still necessary. Anyway, I defer to your decision :-)

@hibariya
Copy link
Contributor Author

That's a good point. Added 🙏

@hibariya hibariya merged commit 6944727 into master May 15, 2025
10 checks passed
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