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

Rails 7.1 #23225

Merged
merged 9 commits into from
Feb 19, 2025
Merged

Rails 7.1 #23225

merged 9 commits into from
Feb 19, 2025

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented Oct 7, 2024

  • Update gem dependencies for rails 7.1
  • Drop legacy connection handler option previously disabled
  • Use new schema migration interface
  • Explicitly set YAML default column serializer to avoid having to set coder everywhere
  • Update Arel::Table usage
  • Allow concurrent-ruby 1.3.5+ (wasn't fixed in rails 7.0)
  • Use new broadcast logging API
  • Load Rails 7.1 defaults with exceptions for tests that fail
  • Waiting on 💚 cross repo after pulling in virtual attributes release.

@jrafanie jrafanie requested a review from Fryguy as a code owner October 7, 2024 22:45
@jrafanie jrafanie changed the title [WIPRails71 [WIP] Rails71 Oct 7, 2024
@jrafanie jrafanie force-pushed the rails71 branch 2 times, most recently from 8888823 to 35c7f80 Compare October 7, 2024 22:53
@jrafanie jrafanie mentioned this pull request Oct 10, 2024
56 tasks
@jrafanie jrafanie force-pushed the rails71 branch 2 times, most recently from 95c915c to 728da06 Compare October 29, 2024 19:37
@jrafanie jrafanie force-pushed the rails71 branch 2 times, most recently from 0eb4c64 to ea2d65d Compare January 7, 2025 23:01
@@ -135,6 +154,8 @@ class Application < Rails::Application

config.autoload_once_paths << Rails.root.join("lib/vmdb/console_methods.rb").to_s

config.active_record.default_column_serializer = YAML if Rails.version >= "7.1"
Copy link
Member

Choose a reason for hiding this comment

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

Does this require a change like #23253 but choosing the serializer? If not, what is the default on 7.1+ that we need to get to?

Copy link
Member Author

@jrafanie jrafanie Feb 18, 2025

Choose a reason for hiding this comment

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

Yes.

https://www.github.com/rails/rails/pull/47422 is the original... in it, he wanted to remove the default of YAML for all applications.

That was scaled back to only do this for new applications, ones that load_defaults 7.1. It's supposed to require a coder specified if you don't have a default.

The one that landed is this one: https://www.github.com/rails/rails/pull/47463

I had that info in a commit message but must have lost it when squashing/rebasing.

Copy link
Member Author

@jrafanie jrafanie Feb 18, 2025

Choose a reason for hiding this comment

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

ok, pushed updated commit message in badad1f

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok, honestly I didn't read any commit messages since you had code comments for a number of things.

@jrafanie jrafanie force-pushed the rails71 branch 4 times, most recently from b1e51a6 to fe33d2a Compare February 18, 2025 23:21
# \
# @service_c1 -> [@vm1]
# \
# @service_c2 -> [@vm1, @vm2]
Copy link
Member Author

Choose a reason for hiding this comment

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

I needed this picture to understand the setup above 😉

service = Service.select(:id, :v_total_vms).find_by(:id => @service.id)
expect(service.v_total_vms).to eq 4
expect(service.attribute_present?(:v_total_vms)).to eq true
#TODO: This is how rails 7.0 worked... not sure why v_total_vms is 1 for @service_c1!
Copy link
Member Author

Choose a reason for hiding this comment

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

🤷

Note, 7.1.5 is the minimum as it's supposed to fix alias_attribute on user
defined methods instead only the generated methods for rails attributes.

It's unclear if it fixes all the issues as you can call alias_attribute
anywhere up the ancestry tree and it's possible lazy loading is preventing
them from being loaded.

See:

https://www.github.com/rails/rails/issues/52820
https://www.github.com/rails/rails/pull/52842
(7.1 backport: https://www.github.com/rails/rails/pull/52844)

Looks like it's still an issue in some situations:
https://www.github.com/rails/rails/issues/50154
https://www.github.com/rails/rails/issues/51717
In ManageIQ#23171, we found that we weren't using the legacy connection handlers
and disabled using them in rails 7.0.  Now, in 7.1, this option has been
removed, so it's safe to remove this option entirely.
Originally, in https://www.github.com/rails/rails/pull/47422,
they wanted to remove the default of YAML for all applications.
It would be nil.

That was scaled back to only do this for new applications. Namely,
ones that call load_defaults 7.1.  It's supposed to require a coder
specified if you don't have a default.

The one that landed is this one: https://www.github.com/rails/rails/pull/47463
Rails 7.1 removed the table_name alias to name so just use name:
See: https://www.github.com/rails/rails/pull/46864

Rails 7.1 removed the writer for the table_alias, but you can
provide an "as" argument to Arel::Table.new, which does an alias.
See: https://www.github.com/rails/rails/pull/48927
This reverts commit e4fa809.

This is not supposed to be needed in rails 7.1
We no longer need to wrap loggers as you can inspect
all broadcasted loggers and automatically send the same
log level or other method calls to each.

We added a wrap method to wrap a logger with a broadcaster in:
ManageIQ/manageiq-loggers#63

See also: https://www.github.com/rails/rails/pull/48615
https://github.com/rails/rails/blob/d437ae311f1b9dc40b442e40eb602e020cec4e49/railties/lib/rails/application/configuration.rb#L92
* belongs_to_required_by_default must be overridden or seeding fails
* Partial inserts cause test failures in ui-classic, content, and amazon provider
* Once we remove unsafe-inline, then we can set this to the default, 0. Since we still use unsafe-inline, we still use X-XSS-Protection.
* Allow deprecations to be found and fixed

Fixes ManageIQ#23172
Note, cache_format_version is backward compatible:
New versions can read old versions.  During rolling deployments
old versions would not be able to read new versions so you must
upgrade code first, then, they'll be able to read old or new versions.

For us, we always deploy code first and restart so changing formats to
the latest will not be a problem.

See also: https://www.github.com/rails/rails/pull/48122
# See: https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#new-activesupport-cache-serialization-format
warn "Warning: Remove redundant config.active_support.cache_format_version = 7.0 from #{__FILE__}:#{__LINE__ + 1} if using config.load_defaults 7.0" if config.active_support.cache_format_version == 7.0
config.active_support.cache_format_version = 7.0

Copy link
Member Author

Choose a reason for hiding this comment

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

From the the commit message:

Note, cache_format_version is backward compatible:
New versions can read old versions. During rolling deployments
old versions would not be able to read new versions so you must
upgrade code first, then, they'll be able to read old or new versions.

For us, we always deploy code first and restart so changing formats to
the latest will not be a problem.

See also: https://www.github.com/rails/rails/pull/48122

@miq-bot
Copy link
Member

miq-bot commented Feb 19, 2025

Checked commits jrafanie/manageiq@13c571e~...9deac23 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint
10 files checked, 1 offense detected

config/application.rb

@jrafanie jrafanie changed the title [WIP] Rails71 Rails 7.1 Feb 19, 2025
@jrafanie jrafanie removed the wip label Feb 19, 2025
Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

🎉

@Fryguy Fryguy merged commit f232310 into ManageIQ:master Feb 19, 2025
8 checks passed
@jrafanie jrafanie deleted the rails71 branch February 19, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants