Fixes #29991 - Enable Zeitwerk #10131
Conversation
ofedoren
force-pushed
the
enable-zeitwerk
branch
from
f2d9ee9
to
a3c648b
Compare
app/services/proxy_status.rb
Outdated
| require_dependency('proxy_status/version') | ||
| require_dependency('proxy_status/tftp') | ||
| require_dependency('proxy_status/puppet_ca') | ||
| require_dependency('proxy_status/logs') |
There was a problem hiding this comment.
If you lazy load the application, are they properly registered? I think that's one worry I had, which mostly affects development environments.
There was a problem hiding this comment.
We have eager loading in dev mode, but yeah, worth checking anyway...
There was a problem hiding this comment.
Looking at https://guides.rubyonrails.org/autoloading_and_reloading_constants.html#single-table-inheritance it says you should turn on eager_load, which we mostly do:
$ rg eager_load config
config/application.rb
152: config.eager_load_paths += ["#{config.root}/lib"]
370: # We use the routes_reloader before the to_prepare and eager_load callbacks
404: dynflow.eager_load_actions!
config/environments/test.rb
16: config.eager_load = false
config/environments/production.rb
7: config.eager_load = true
config/environments/development.rb
10: config.eager_load = truePerhaps this explains why you saw it work in development, but not in tests?
| @@ -1,6 +1,6 @@ | |||
| require 'test_helper' | |||
|
|
|||
| class HttpProxyTest < ActiveSupport::TestCase | |||
| class Foreman::HttpProxyTest < ActiveSupport::TestCase | |||
There was a problem hiding this comment.
Why does this need to be in the Foreman namespace? Because it's in the foreman directory in tests?
There was a problem hiding this comment.
Mocha/minitest (not sure) has its own logic for constant lookups and probably due eager loading it treats this class as if it was test/unit/http_proxy_test.rb. It finds the same constant HttpProxy, which mistreats the one with additional Foreman:: scope.
| @@ -1,5 +1,5 @@ | |||
| module Foreman | |||
| module HTTPProxy | |||
| module HttpProxy | |||
There was a problem hiding this comment.
Perhaps HTTP needs to be its own acronym as well
There was a problem hiding this comment.
Could you then update #10076? For some reason though, I didn't make this an acronym in my alternative. Maybe there are more pitfalls than it seems...
There was a problem hiding this comment.
I'm going through open issues on https://projects.theforeman.org/issues/29991 and highlighted a few that are open and I think are related.
I'm not sure https://projects.theforeman.org/issues/33975 is still an issue.
|
|
||
| Rails.application.config.before_initialize do | ||
| # load topbar | ||
| Menu::Loader.load | ||
| end | ||
|
|
||
| Foreman.settings.load_definitions |
There was a problem hiding this comment.
Would this part address https://projects.theforeman.org/issues/33888?
There was a problem hiding this comment.
I think so, it seems this gets executed as part of https://github.com/theforeman/foreman/blob/develop/config/initializers/foreman.rb#L37 anyway. It also contains Foreman.settings.load_values, which is called in after_initialize callback. I think that is redundant...
I mean, I'm not 100% sure why we have it as we do, but for me it seems that Foreman.settings.load in to_prepare is sufficient and does the same thing as combination of Foreman.settings.load_definitions here and Foreman.settings.load_values in after_initialize. For me it also seems like currently we try to load the definitions and values 2 times.
Should I change the commit message for this?
There was a problem hiding this comment.
I'll leave it for now here as is, 'cause I didn't yet tested these new changes with plugins (working on it though).
lib/fog_extensions.rb
Outdated
There was a problem hiding this comment.
Would this address https://projects.theforeman.org/issues/33889?
There was a problem hiding this comment.
It should, since it get's only loaded explicitly in https://github.com/theforeman/foreman/blob/develop/config/application.rb#L385.
Should I change the commit message?
There was a problem hiding this comment.
Please. And if it can be merged today without the whole Zeitwerk changes, then submitting it as its own PR will be great.
Nope, that was a few different issues |
Currently I'm trying to clean things up a bit. Per my thinking (and hopefully Rails') Such (auto|re)loadable code, which is not a concern/model/controller/etc I've moved into Although, I still think that we far away from proper (auto|re)loading for dev purposes. As of now I'm more in scope of making Zeitwerk work, maybe someone in the future will have time to refactor a bit, so we can maybe disable eager loading and use reloading in other places than concerns and models (those I think still work as expected). |
Zeitwerk uses constant to snake case to resolve the paths. If we want to change some constants to be Uppercase, we need to use the inflector.
Based on #10076 (included). Added some more Puppetca -> PuppetCA renaming since we added a new acronym anyway.
Some
require_dependencycalls were removed, since with Zeitwerk there is no need in them (AFAIK they also don't work as with Rails' loader), some were changed torequire(I think we can change them torequire_relative).I was able to load the application with Ansible, Bootdisk, Discovery, OpenSCAP, Puppet, REX, Tasks, Webhooks, Virt-who-config, Katello plugins (they do need some tweaks though and I'm planning to create a PR to each plugin I tested with) and successfully provision a host on libvirt with content management and SCAP scans.
There are still some issues like weird(?)
requirestatements, non-conventional structure, etc, but we can deal with them later if we want to. It doesn't seem to be necessary anyway. What is necessary though is that we need to get rid ofDEPRECATION WARNING: Initialization autoloaded the constantswarning. It's huge, annoying and may become an error in the future.Not sure now how to do so, but can be done in a separate PRdone here.