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

Add Rails 7 Support #7235

Merged
merged 5 commits into from
Mar 7, 2022
Merged

Conversation

tagliala
Copy link
Contributor

@tagliala tagliala commented Dec 21, 2021

Hi, this is a work in progress and I'm opening this PR as a reference to #7196

I'm trying to update activeadmin to use Rails 7, but there are some blocking dependencies:

✅ No known issues

✅ Dependencies checklist

✅ Removed dependencies

At the moment, there are no stable JRuby versions supporting MRI 2.7, the minimum required version to run Rails 7.0. There was too much effort in trying to build against jruby-head, so JRuby dependency has been temporarily removed

Test in your application

1. Changes to Gemfile

Add the following to your Gemfile:

gem 'activeadmin', github: 'activeadmin/activeadmin'

Tests

  • I cannot replicate the zeitwerk failure I see on CI on my local development machine (specs are green)
  • I have cucumber related failures because of uninitialized constant #<Class:Cucumber::Rails::Database>::NullStrategy (NameError). It is worth to mention that cucumber-rails does seem to work with Rails 7, or at least it does not have specs covering this issue. Ref: Support for Rails 7 cucumber/cucumber-rails#523 (comment)

@tagliala tagliala mentioned this pull request Dec 21, 2021
@tagliala tagliala force-pushed the feature/railties-7 branch 3 times, most recently from 3d78c26 to 7253b1f Compare December 24, 2021 11:39
@tagliala
Copy link
Contributor Author

tagliala commented Dec 24, 2021

Updates:

| An error occurred while loading ./spec/requests/default_namespace_spec.rb.
| Failure/Error: require "#{ActiveAdmin::TestApplication.new.full_app_dir}/config/environment.rb"
| 
| Zeitwerk::NameError:
|   expected file ~/dev/activeadmin/tmp/test_apps/rails_70/app/admin/admin_users.rb to define constant AdminUsers, but didn't

I can replicate this issue locally when I try to run CI workflows using act, but if I login on the docker container and I try there, specs pass. Any hint would help

@tagliala
Copy link
Contributor Author

tagliala commented Dec 24, 2021

🟢 Specs are green!

I've got the reason of the failure (and - of course - it was pretty simple)

Rails 7's default config/environments/test.rb has the following change:

config.eager_load = ENV["CI"].present?

So Rails 7 is going to perform eager loading and it fails, because remove_active_admin_load_paths_from_rails_autoload_and_eager_load does not prevent eager loading of classes in that folder

I'm not sure of the fix (disable eager loading), please let me know what do you think

@tagliala tagliala force-pushed the feature/railties-7 branch 3 times, most recently from 6791aa9 to 9469604 Compare December 26, 2021 10:22
@tagliala
Copy link
Contributor Author

tagliala commented Dec 26, 2021

UPDATE

Ransack 2.5.0 released, four to go:

  • arbre (pending release)
  • cucumber-rails
  • db-query-matchers (pending merge and release)
  • inherited_resources (pending release)

Cucumber Rails may need help, there are two conditional test cases to write --> cucumber/cucumber-rails#523

@tagliala
Copy link
Contributor Author

tagliala commented Dec 30, 2021

UPDATE 2021-12-30

There is an issue with Rails 7, code reloading, dev environment, and the new server_timing option not detected by specs (because server_timing is only enabled in development environment?)

It is described here: #7196 (comment)

It can be replicated in dev environment by changing something in an activeadmin file (/app/admin/dashboard.rb) and reloading the page

Changing server_timing to false is a workaround, but this issue should be fixed. Any help would be appreciated

Ref: rails/rails@0547b16

rails/rails#36289


UPDATE 2021-12-31

The problem is an incompatibility with the new Server-Timing feature

Server-Timing subscribes to all notifications, and it creates an event by using args. In our case, args is an array of two elements containing ['active_admin.application.before_load', ActiveAdmin::Application], but Event expects five parameters

rails/rails@0547b16#diff-7bae50494146f858329a92e06ec56eb449abff287fc6de733b99b11c589b6c3aR16

I do not know how to fix this

@alejandroperea
Copy link
Contributor

@tagliala this PR has been merged #7232. It replaces the db-query-matchers gem with an Rspec matcher that is doing the same check than the gem.

Thanks a lot for your effort!

@tagliala
Copy link
Contributor Author

Hi @alejandroperea, thanks!

I was hoping for #7275 to be merged before rebasing, so I can clean up other things and remove unrelated changes and gem updates

@javierjulio
Copy link
Member

@tagliala you're good to go as #7275 is merged. I'll approve and run CI here once you push up that update. Thanks again!

@tagliala tagliala changed the title [Rails 7 WIP] Add Rails 7 Support (missing jruby support) Add Rails 7 Support Mar 5, 2022
@tagliala tagliala marked this pull request as ready for review March 5, 2022 14:04
Copy link
Member

@deivid-rodriguez deivid-rodriguez left a comment

Choose a reason for hiding this comment

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

Looks good to me! 🎉

@mgrunberg
Copy link
Contributor

@tagliala cucumber-rails 2.5.0 has been released. I created #7369 to upgrade master and tagliala#7 to upgrade this PR in case 7369 reaches master first.

@tagliala
Copy link
Contributor Author

tagliala commented Mar 7, 2022

@mgrunberg thanks

I can rebase if #7369 is going to be merged before this one

@deivid-rodriguez
Copy link
Member

I don't have a preference either way, I'll let @javierjulio choose when he gets to this :)

@ghost
Copy link

ghost commented Mar 7, 2022

@tagliala Thank you for your efforts getting this updated!

@javierjulio
Copy link
Member

@tagliala we just merged #7369 so this is ready for rebasing. I'll merge this next. Thank you again! 🙌🏻

mgrunberg and others added 5 commits March 7, 2022 15:56
- Relax railties dependency to allow 7.0
- Move Rails 6.1 Gemfile to gemfiles directory
- Set main test application to Rails 7.0
This method has been removed in Rails 7 and replaced with `constantize`
`ActiveAdmin::Aplication.remove_active_admin_load_paths_from_rails_autoload_and_eager_load`
is not working on feature tests.

Starting from Rails 7.0, running tests with eager_load enabled produces the following error:

```
Failure/Error: require
"#{ActiveAdmin::TestApplication.new.full_app_dir}/config/environment.rb"

FrozenError:
  can't modify frozen Array: ["<root>/tmp/test_apps/rails_70/app/admin",
"<root>/tmp/test_apps/rails_70/app/channels",
"<root>/tmp/test_apps/rails_70/app/controllers",
"<root>/tmp/test_apps/rails_70/app/controllers/concerns",
"<root>/tmp/test_apps/rails_70/app/helpers",
"<root>/tmp/test_apps/rails_70/app/jobs",
"<root>/tmp/test_apps/rails_70/app/mailers",
"<root>/tmp/test_apps/rails_70/app/models",
"<root>/tmp/test_apps/rails_70/app/models/concerns",
"<root>/tmp/test_apps/rails_70/app/policies"
```

`ActiveAdmin::Aplication.remove_active_admin_load_paths_from_rails_autoload_and_eager_load`
fails because `ActiveAdmin.application.load_paths` value is
`<root>/app/admin` when it should be `<root>/tmp/test_apps/rails_70/app/admin`.

`ActiveAdmin::ApplicationSettings` initialize `load_path` with
`[File.expand_path("app/admin", Rails.root)]`.

On testing app bootstrap ([this line](https://github.com/activeadmin/activeadmin/blob/master/features/support/env.rb#L12))
`Rails.root` is nil and `File.expand_path` is expanding to the folder where
`rake test` runs (`<root>`).

`ActiveAdmin::Engine` has an initializer to define default value for
`load_paths`. The problem is that the block runs after initializers in
`config/initializers` folder, including `ActiveAdmin.setup` where
`ActiveAdmin::Aplication.remove_active_admin_load_paths_from_rails_autoload_and_eager_load`
is called from.

This commit makes sure the block runs before `config/initializers`.

That way the block sets a proper default.
At the moment of this commit, JRuby does not support Ruby 2.7, the
minimum version required to run Rails 7.0, and gems did not install
with jruby-head

This platform can be added back when JRuby will release a version
compatible with 2.7 MRI
@tagliala
Copy link
Contributor Author

tagliala commented Mar 7, 2022

Got a segmentation fault on 2.7 vs 6.1. Any chance to restart the build? :\

@javierjulio
Copy link
Member

Yes I will, thanks for checking it as I was about to ask whether we needed a rebuild or not.

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

Thank you @tagliala! 🙌🏻❤️

@javierjulio javierjulio merged commit 930f0f1 into activeadmin:master Mar 7, 2022
@deivid-rodriguez
Copy link
Member

Thanks so much @tagliala and @mgrunberg!!

@tagliala tagliala deleted the feature/railties-7 branch March 7, 2022 15:41
@grantspeelman
Copy link

Fantastic Work 🥳

@dwaynemac
Copy link

This is awesome. Thank you very much

@christopherdbull
Copy link

I've updated to Rails 7 but am getting crashes with uninitialized model constants on the first line of our Admin declarations. Using version 2.11.1, just wondering if you had any hints to debug? Not doing anything fancy with loading as far as I can see, but zeitwerk seems unhappy...

@tierra
Copy link

tierra commented Apr 7, 2022

Were you using classic mode before? (as that was removed in Rails 7)

Have you checked this?

bin/rails zeitwerk:check

You might want to review the Classic to Zeitwerk HOWTO

Anecdotally, I've finished upgrading my app (using ActiveAdmin) to Rails 7 recently, using this update, and it went smoothly. No issues, and no additional AA changes required, it just worked.

@yosiat
Copy link

yosiat commented Feb 7, 2023

Thanks for adding support for rails 7!

Is it possible to release a version for this PR ?

@tagliala
Copy link
Contributor Author

tagliala commented Feb 7, 2023

Hi @yosiat , Rails 7 support has been released in v2.11.0

Changelog: https://github.com/activeadmin/activeadmin/releases/tag/v2.11.0

@yosiat
Copy link

yosiat commented Feb 7, 2023

@tagliala sorry, my mistake!

@jgmontoya jgmontoya mentioned this pull request Oct 6, 2023
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.

None yet