Run tests against Ruby 3.3 - #11767
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ofedoren
left a comment
There was a problem hiding this comment.
Thanks, @jturel, it's nice to see green CI here without any changes needed :) Although, CI might not catch all :/
Anyway, I only have one concern: we need to run on both Ruby 3.0 and Ruby 3.3. The Set became part of standard lib in Ruby 3.2 only, so 3.0 still requires us to explicitly require 'set' . Even though it seems fine, I'd rather be paranoid and add those statements where we actually call Set.new (app/services/katello/product_content_finder.rb#L29, app/services/katello/registration_manager.rb#L71) or maybe include it somewhere else, so we require it before it gets used.
| s.email = ["ericdhelms@gmail.com", "walden@redhat.com"] | ||
| s.homepage = "https://github.com/Katello/bastion" | ||
| s.license = "GPL-2.0-or-later" | ||
| s.gem.required_ruby_version = ['>= 2.5.0', '< 2.7.0'] |
There was a problem hiding this comment.
Not related to this PR, but that's weird. Is .gem. really needed? All the other gemspecs have s.required_ruby_version...
There was a problem hiding this comment.
Pretty sure we don't need the gemspec at all since bastion is now loaded as an engine and we don't build it as a gem..
|
Or, since those are not necessary changes, we can leave those statements (they are harmless), but add TODO comment, something like |
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
What are the testing steps for this pull request?