Fixes #39361 - Add Ruby 3.3 support to Foreman#11004
Conversation
| erb.location = source_name, 0 | ||
| erb.result(get_binding) | ||
| rescue ::SyntaxError => e | ||
| new_e = SyntaxError.new(name: source_name, message: e.message) |
There was a problem hiding this comment.
This is not necessary, just so it's more obvious what we raise and expect.
| assert_equal '2'.to_json, @response.body | ||
|
|
||
| post :preview, params: { :template => '<%= 1+ -%>', :id => template }, session: set_session_user | ||
| assert_includes @response.body, 'parse error on value' |
There was a problem hiding this comment.
The message is now different, I decided to use a small, but noticable fragment.
| inherit_from: .rubocop_todo.yml | ||
|
|
||
| AllCops: | ||
| TargetRubyVersion: 2.7 |
There was a problem hiding this comment.
This can be extracted, but since we're at it...
| gem 'validates_lengths_from_database', '~> 0.5' | ||
| gem 'friendly_id', '>= 5.4.2', '< 6' | ||
| gem 'secure_headers', '>= 6.3', '< 8' | ||
| gem 'safemode', '>= 1.4', '< 2' |
There was a problem hiding this comment.
Before 2.0 safemode was not Ruby 3.3 ready.
There was a problem hiding this comment.
Also, packit failure is expected, we need a PR to foreman-packaging which bumps the version for rpm/devel
There was a problem hiding this comment.
Does it make sense to split safemode 2 into its own PR? I don't think there's any blocker to merging that, other than some coordination with packaging.
| box = Safemode::Box.new(scope, allowed_helpers, source_name) | ||
| erb = ERB.new(source_content, trim_mode: '-') | ||
| box.eval(erb.src, allowed_variables) | ||
| rescue ::Racc::ParseError => e |
There was a problem hiding this comment.
This is due to changes of underlying parser in safemode.
|
Based on https://community.theforeman.org/t/ruby-3-3-support-across-the-foreman-ecosystem/46602/6 we keep this open till 04.06.2026 and then should have a better picture with how to proceed. |
ofedoren
force-pushed
the
feat-39361-ruby-3.3
branch
from
59cd415 to
9ffd17d
Compare
| # We only save a value into the image_file field if the value is not the default path, (which was placed in the entry when it was displayed,) | ||
| # and it is not a directory, (ends in /) | ||
| value = ((default_image_file == file) || (file.to_s =~ /\/\Z/) || file == "") ? nil : file | ||
| value = ((default_image_file == file) || /\/\Z/.match?(file) || file == "") ? nil : file |
There was a problem hiding this comment.
Is this equivalent to:
| value = ((default_image_file == file) || /\/\Z/.match?(file) || file == "") ? nil : file | |
| value = (default_image_file == file || file.end_with?('/') || file.empty?) ? nil : file |
| is_int = (value =~ /\A[-+]?\d+\z/) || value.is_a?(Integer) | ||
| is_int = value.to_s.match?(/\A[-+]?\d+\z/) || value.is_a?(Integer) | ||
| is_pg = ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'postgresql' | ||
| # Once Postgresql 8 support is removed (used in CentOS 6), this could be replaced to only keep the first form (working well with PG 9) |
| def cast_facts(table, key, operator, value) | ||
| is_int = (value =~ /\A[-+]?\d+\z/) || value.is_a?(Integer) | ||
| is_int = value.to_s.match?(/\A[-+]?\d+\z/) || value.is_a?(Integer) | ||
| is_pg = ActiveRecord::Base.connection.adapter_name.downcase.starts_with? 'postgresql' |
There was a problem hiding this comment.
Fun fact: we dropped non-PG in Foreman 2.0.
There was a problem hiding this comment.
I've dropped that. Also it now seems that we don't need || value.is_a?(Integer), so dropped that as well.
ofedoren
force-pushed
the
feat-39361-ruby-3.3
branch
from
0e95553 to
60d6b7e
Compare
4 participants
From my local testing it seems we don't need much, thus opening as is to see what CI thinks of it.
Also, please note: we don't drop Ruby 3.0 support, so don't treat this PR as "we can finally use Ruby 3.1-3 new features". We can't :/