-
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
Fixes #20908 - postpone initialization of Dynflow runtime #4826
Fixes #20908 - postpone initialization of Dynflow runtime #4826
Conversation
Issues: #20908 |
bffe93e
to
7cdaf5a
Compare
In #18618, the initialization code was moved from foreman-tasks to foreman. There was however one thing done differently, where the initialization of dynflow world was moved directly to the time `Rails.application.dynflow` was called. This is not right, as there are situations where plugins go and call `Rails.application.dynflow.config` before the whole stack is prepared, which leads to database connection being opened too early, which can cause the whole lot of issues (as we've hit currently with Katello). There was also change in the signature of `Dynflow::Rails#initialize` method, that needs to be updated.
7cdaf5a
to
43043cf
Compare
@@ -246,12 +246,27 @@ def dynflow | |||
if defined?(ForemanTasks) | |||
ForemanTasks.dynflow | |||
else | |||
Dynflow::Rails.new(Foreman::Dynflow::Configuration.new) | |||
::Dynflow::Rails.new(nil, Foreman::Dynflow::Configuration.new) |
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.
Dynflow::Rails
was resolving to Foreman::Dynlfow::Rails
which was not existing: the after_initialize block actually discovered this issue as it seems this code-path was not run in tests before.
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, I had to do something similar in #4717, can confirm it fixes the problem
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.
Yes, this should fix the issue you were facing there + making it work with later dynflow version as well that came with Dynflow/dynflow@ff129a6#diff-56fbff8c6eb846b55588d78db914924aR10
And all the tests are green again :) |
Thanks @iNecas - this should go into 1.16 I suppose? |
yes please |
Cherry-picked as 7ad59f2 |
In #18618, the initialization code was moved from foreman-tasks to
foreman. There was however one thing done differently, where the
initialization of dynflow world was moved directly to the time
Rails.application.dynflow
was called. This is not right, as there aresituations where plugins go and call
Rails.application.dynflow.config
before the whole stack is prepared, which leads to database connection
being opened too early, which can cause the whole lot of issues (as
we've hit currently with Katello).
There was also change in the signature of
Dynflow::Rails#initialize
method, that needs to be updated.