-
Notifications
You must be signed in to change notification settings - Fork 987
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
Load Menu::Loader in 'befor_init' #8938
Load Menu::Loader in 'befor_init' #8938
Conversation
Issues: #33884 |
Can we have a subtask for the RM 33886? Or use Refs #33886, but this is not the fix for the RM it is referencing :) |
a492fdf
to
e41cc30
Compare
Yop, correct RM: https://projects.theforeman.org/issues/33964 |
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.
The menu doesn't work now, I believe it is the special case where we should use before_initialize
(just because we abuse the autoload_once_paths
for it).
config/initializers/foreman.rb
Outdated
Foreman::Plugin.initialize_default_registries | ||
Foreman::Plugin.medium_providers_registry.register MediumProviders::Default | ||
|
||
Rails.application.config.to_prepare do | ||
# load topbar | ||
Menu::Loader.load |
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.
The menu disappears for me with this change.
Though it's wrong even conceptually I believe, Menu::Manager
is in registries folder, which is autoload_once (never reloaded). This is probably wrong and we should reload menu on code reload, but we do not, so it would load the menu items multiple times.
So IMHO, this is a candidate for before_initialize
, because it has already initialized the Rails.autoloaders.once
, which holds the Menu constant. Check the bootstrap.rb
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.
The menu disappears for me with this change.
Really? Because it works for me fine (on vanilla foreman, no plugins)
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.
Though it's wrong even conceptually I believe, Menu::Manager is in registries folder, which is autoload_once (never reloaded). This is probably wrong and we should reload menu on code reload, but we do not, so it would load the menu items multiple times.
Maybe we should move it to services and reload it on every change
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.
Yeah, when you add plugins into the mix it stop working, because the root menus got initialized from plugins too soon and Foreman won't reinitialize them properly.
Maybe we should move it to services and reload it on every change
I believe we should, there is no real reason to initialize it soon, but that's a bit of a challange to do it correctly as there might be 🐉s :)
If you're up for implementing it I'm all for it, but putting it into before_initialize
is acceptable workaround for me :)
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.
For now I put Menu::Loader.load
to before_initialize
, for follow up refactoring I created https://projects.theforeman.org/issues/33975 so if there will be time I'll look into it
|
||
# clear our users topbar cache | ||
# The users table may not be exist during initial migration of the database | ||
TopbarSweeper.expire_cache_all_users if (User.table_exists? rescue false) |
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.
Probably won't hurt to delete the cache on every code reload 🤷
So I guess 👍 on this
e41cc30
to
05daca8
Compare
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.
Thanks @stejskalleos !
No description provided.