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

Fixes #37847 - Support Rails 7.0 #921

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

ofedoren
Copy link
Member

No description provided.

app.reloader.to_prepare do
ForemanTasks.dynflow.require!
ForemanTasks.dynflow.config.queues.add(DYNFLOW_QUEUE, :pool_size => Setting['remote_execution_workers_pool_size']) if Setting.table_exists? rescue(false)
ForemanTasks.dynflow.config.eager_load_paths << File.join(ForemanRemoteExecution::Engine.root, 'app/lib/actions')
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this is not in a reloader.to_prepare block? Doing this each time the application is reloaded would most probably lead to memory bloating? Unless the list of paths to eager load get wiped on reload

Copy link
Member Author

Choose a reason for hiding this comment

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

For a long time, Zeitwerk didn't like if during initialization time an autoloadable constant was addressed. In this case Setting, which is a model constant, thus an autoloadable/reloadable constant.

But now it seems redundant to put this into that block...

@ofedoren ofedoren force-pushed the feat-37847-support-rails-7 branch from deb8e7f to 94c40be Compare October 21, 2024 10:57
@ofedoren ofedoren force-pushed the feat-37847-support-rails-7 branch from 94c40be to 9f12df2 Compare October 22, 2024 11:29
@ofedoren
Copy link
Member Author

As of now, this PR can be merged at any time it's ready (dropped the last commit) since it doesn't depend on Foreman's PR.

Copy link
Contributor

@adamruzicka adamruzicka left a comment

Choose a reason for hiding this comment

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

LGTM

@adamruzicka adamruzicka merged commit 8b169c2 into theforeman:master Oct 22, 2024
22 checks passed
@adamruzicka
Copy link
Contributor

Thank you @ofedoren !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants