varvet / godmin

Admin framework for Rails 5+
http://godmin-sandbox.herokuapp.com
MIT License
486 stars 47 forks source link

Rails 7 support #267

Closed anderscarling closed 2 years ago

anderscarling commented 2 years ago

Sets up gem files for running godmin under rails 7 and fixes test issues related to rails 7.

Note that I've only confirmed that this all works through the test suite and through running the test/dummy app, I've not actually set it up with a "real" project on rails 7.

Issue 1: Godmin::Resolver#_find_all was broken as ActionView::FileSystemResolver#query

was removed in rails 7.

However, our implementation of Godmin::Resolver#_find_all was really just a loop around the original _find_all methods implementation, but with one of the original arguments (prefix) substituted in each loop.

Thus we can rely on calls to super with the prefix argument to get the job done.

Issue 2: Rails 7 defaults to eager loading dependencies in test env when ‘CI’ env variable is set.

This causes the test/dummy/admin engine to be eager loaded in the generated app used by ResourceGeneratorTest. This causes issues as test/dummy/admin relies on models in the test/dummy app and thus shouldn't be loaded outside of it.

anderscarling commented 2 years ago

Weirdly, I don't get these issues in my local env. Will investigate.

anderscarling commented 2 years ago

The build issues are due to new projects under rails 7 defaulting to eager load code in test env if CI env variable is present, investigating a workaround.

anderscarling commented 2 years ago

I pushed a somewhat hacky change that will hopefully adress the builds issue, not sure if I like it but had a hard time finding a solid workaround for the issue.

(I even tried rewriting test/tmp/config/environments/test.rb, which fixed the error for one test case, but the engine based test case in ResourceGeneratorTest still hit the issue when invoking test/tmp/fakemin/bin/rails - supposedly because it does not use the parent application configuration).

dgmstuart commented 2 years ago

Oh, is CI now effectively a 'reserved' environment variable? Would it make sense to just use a different name? Or is it set by default on Travis?

anderscarling commented 2 years ago

Oh, is CI now effectively a 'reserved' environment variable? Would it make sense to just use a different name? Or is it set by default on Travis?

I think it's set by default on Travis, at least there is no reference to it in .travis.yml