-
Notifications
You must be signed in to change notification settings - Fork 991
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
Fixes #37825 - Upgrade Rails to 7.0 #10299
base: develop
Are you sure you want to change the base?
Changes from all commits
8012399
21c1b11
c7a9951
188b129
b5e9f73
cf723dd
4d9f64c
1a151be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,4 +10,8 @@ def safe_value | |
def hidden_value | ||
HIDDEN_VALUE | ||
end | ||
|
||
def hidden_value? | ||
attributes['hidden_value'] | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,7 @@ class Foreman::Consoletie < Rails::Railtie | |
|
||
module Foreman | ||
class Application < Rails::Application | ||
config.load_defaults '6.1' | ||
config.load_defaults '7.0' | ||
|
||
# Rails 5.0 changed this to true, but a lot of code depends on this | ||
config.active_record.belongs_to_required_by_default = false | ||
|
@@ -99,11 +99,17 @@ class Application < Rails::Application | |
config.active_support.use_authenticated_message_encryption = false | ||
config.action_dispatch.use_authenticated_cookie_encryption = false | ||
|
||
# Rails 6.0 changed this to :zeitwerk | ||
config.autoloader = :zeitwerk | ||
|
||
# Rails 6.1 changed this to true, but apparently our codebase is not ready for bidirectional associations | ||
config.active_record.has_many_inversing = false | ||
|
||
# Rails 7.0 changed this to true | ||
config.active_record.verify_foreign_keys_for_fixtures = false | ||
config.active_record.automatic_scope_inversing = false | ||
# Rails 7.0 changed this to OpenSSL::Digest::SHA256 | ||
# We'd probably need a rotator: | ||
# https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256 | ||
config.active_support.hash_digest_class = OpenSSL::Digest::SHA1 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is tricky one, I've noticed that only when Katello started failing with some hashes not being matched. Although, this is safe for now, I'd say we need to migrate from SHA1 since RHEL9 won't support it for cert signing purposes at least: https://bugs.ruby-lang.org/issues/19307 If we change this now, we'd probably need that (https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256) thingy as well as some changes in Katello since I've noticed it heavily utilizes https://github.com/Katello/katello/blob/master/app/lib/katello/util/data.rb#L9 for creating some labels/ids? https://github.com/Katello/katello/blob/master/app/models/katello/content_view_environment.rb#L117, which will break with SHA256. Although, we can fix it in Katello itself. |
||
|
||
# Setup additional routes by loading all routes file from routes directory | ||
Dir["#{Rails.root}/config/routes/**/*.rb"].each do |route_file| | ||
config.paths['config/routes.rb'] << route_file | ||
|
@@ -113,6 +119,8 @@ class Application < Rails::Application | |
# Application configuration should go into files in config/initializers | ||
# -- all .rb files in that directory are automatically loaded. | ||
|
||
# Recommended by Rails: https://guides.rubyonrails.org/v7.0/configuring.html#config-add-autoload-paths-to-load-path | ||
config.add_autoload_paths_to_load_path = false | ||
# Autoloading | ||
config.autoload_paths += %W(#{config.root}/app/models/auth_sources) | ||
config.autoload_paths += %W(#{config.root}/app/models/compute_resources) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,8 @@ | |
# test suite. You never need to work with it otherwise. Remember that | ||
# your test database is "scratch space" for the test suite and is wiped | ||
# and recreated between test runs. Don't rely on the data there! | ||
config.cache_classes = true | ||
# Disable reloading for integration tests | ||
config.cache_classes = ARGV.grep(/test\/integration/).any? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be the other way around? Only enable it when it's Also, https://github.com/rails/spring?tab=readme-ov-file#enable-reloading notes it needs to be Another note: https://github.com/rails/spring?tab=readme-ov-file#temporarily-disabling-spring says:
|
||
|
||
config.eager_load = true | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,4 @@ | ||
require 'test_helper' | ||
require 'foreman/cast' | ||
|
||
class CastTest < ActiveSupport::TestCase | ||
include Foreman::Cast | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -92,7 +92,7 @@ class HasManyCommonTest < ActiveSupport::TestCase | |
|
||
## Test name methods resolve for Plugin AR objects | ||
class ::FakePlugin; end | ||
class ::FakePlugin::FakeModel; end | ||
class ::FakePlugin::FakeModel < ApplicationRecord; end | ||
Comment on lines
94
to
+95
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this even be module FakePlugin
class FakeModel < ApplicationRecord
end
end |
||
|
||
test "should return plugin associations" do | ||
Host::Managed.class_eval do | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,3 @@ | ||
require 'foreman/access_control' | ||
|
||
module AccessPermissionsTestBase | ||
extend ActiveSupport::Concern | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this was the whole problem? That it set an instance variable on the class? That looks nasty and I wonder how that worked in the past.
From reading the Rails source I get the impression should handle this itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems so. I couldn't find a reason why we did that at the first place, so I hope removing doesn't break anything.
I'm still going to try basic processes (CRUDing a virtual host, running jobs, some Katello-related stuff, oscap, etc) to verify PRs after everything open is green.