-
Notifications
You must be signed in to change notification settings - Fork 988
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 #29991 - Enable Zeitwerk #10131
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have eager loading in dev mode, but yeah, worth checking anyway...
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.
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 = true
Perhaps this explains why you saw it work in development, but not in tests?
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.
What's the logic we should keep? Nothing in lib
and everything in app/lib
?
@@ -1,6 +1,6 @@ | |||
require 'test_helper' | |||
|
|||
class HttpProxyTest < ActiveSupport::TestCase | |||
class Foreman::HttpProxyTest < ActiveSupport::TestCase |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps HTTP needs to be its own acronym as well
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this part address https://projects.theforeman.org/issues/33888?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like #10131 (comment).
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this address https://projects.theforeman.org/issues/33889?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
@ofedoren I think we should also update all plugins that need changes should update the required Foreman version to 3.12. That should make sure packaging picks it up and prevent accidental cherry picks. |
255706e
to
de2e48f
Compare
c4554d4
to
b350a9b
Compare
# E.g. Base.register_type(BMC) | ||
# Some constants that use such classes may be defined before all the related classes/models are loaded and registered | ||
# E.g. InterfaceTypeMapper::ALLOWED_TYPE_NAMES | ||
Rails.application.reloader.to_prepare do |
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.
This certainly looks cleaner, but what's the difference to Rails.application.config.to_prepare
? If a plugin provides an implementation to any STI (foreman_discovery's Host::Discovered
comes to mind), should they do this too and if so, do we have the right hooks in place?
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.
I'm note that sure :/
TL;DR: I've noticed that config.to_prepare
blocks run too soon
Currently we do really nasty things (to my taste). For example: Rails during startup runs files under config/initializers
, where we do some configurations, which is ok. But we for some reason love to call classes that should be loaded as application
or after basics are done (gem/rails configs). We have core
and plugins
, which is like chicken-egg problem. Plugins rely on the core, they don't do anything without it, but they are being loaded before the core due to them being a dependency, just like any other gem. AFAIU, to make the application work, all the deps must be loaded and configured, then the app code is loaded and starts to serve requests. During plugin loading, config.to_prepare
is being called after all config/initializers
are done, but before eager_loading
(which supposedly should load app/**). In these config.to_prepare
blocks we refer a model or controller before it even got loaded by the application itself, so the constant gets loaded, but without additional context. If eager loading of the app was done earlier, this wouldn't be a problem, since everything under e.g. app/models/nic would be loaded and all the nic types would be registered. Here's an example which makes me feel stupid (or maybe proving it 😄 ) since I'm still not sure what should we do:
https://github.com/theforeman/foreman_remote_execution/blob/master/lib/foreman_remote_execution/engine.rb#L341
This only line (I'm sure we have many more similar cases) is a headache for me:
- Rails/bundler loads plugins before Foreman. At least
to_prepare
blocks from plugins are being run before we run such block in Foreman. In other words: this (https://github.com/theforeman/foreman_remote_execution/blob/master/lib/foreman_remote_execution/engine.rb#L310) is run before this (https://github.com/theforeman/foreman/blob/develop/config/initializers/foreman.rb#L32). - In REX's
to_prepare
block there is invocation ofApi::V2::InterfacesController
class, which gets autoloaded(?) on demand. Since this is the first time the constant is being referenced, it gets parsed and loaded. - In this class there is a reference to another constant --
InterfaceTypeMapper::ALLOWED_TYPE_NAMES
(https://github.com/theforeman/foreman/blob/develop/app/controllers/api/v2/interfaces_controller.rb#L37). This makesInterfaceTypeMapper::ALLOWED_TYPE_NAMES
to be parsed and loaded (since this is the first time it's being referenced). - In order to load that constant, there is an invocation of another constant --
Nic::Base
(https://github.com/theforeman/foreman/blob/develop/app/services/interface_type_mapper.rb#L5). The constant is loaded, butNic::Base.allowed_types
won't return correct value unless other NIC types are also loaded. Thus, we need to preload these models. - I've tried different ways and places, but got
DEPRECATION WARNING: Initialization autoloaded the constants: ...
. As the message suggested, early referenced constants must be referenced inRails.application.reloader.to_prepare
block. Note that this usesreloader
instead ofconfig
.
P.S. Currently we load NIC types by calling require_dependency
at the end of model definitions. But this is a deprecated method, so we shouldn't use these. We could change them to require
, but what's the point of zeitwerk if we still need that for model files. Rails are aware of STI issues and there are suggestions how to handle it, but none of them work for us: enable eager loading
- done, but happens too late; poke db to get types
-- tried, but didn't work in test env.
c17812e
to
c10f756
Compare
c10f756
to
8ca9e3e
Compare
8ca9e3e
to
581d4b8
Compare
7028464
to
9bc2128
Compare
Based on #10076 (included). Added some more Puppetca -> PuppetCA renaming since we added a new acronym anyway.
Some
require_dependency
calls 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(?)
require
statements, 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 constants
warning. 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.