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.

Sign up for GitHub

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

Jump to bottom

Fixes #32729 - reload setting prior run #636

Merged
merged 2 commits into from
Jun 10, 2021
Merged

Fixes #32729 - reload setting prior run #636

merged 2 commits into from
Jun 10, 2021

Conversation

ezr-ondrej
Copy link
Member

In Foreman we've switched to in-memory setting cache, that needs to be
reloaded on demand, Rails is doing that on every request, for dynflow
actions we need to do it on every phase.

@adamruzicka
Copy link
Contributor

Looks good to me. @ShimShtein could you take this for a spin?

@ShimShtein
Copy link
Member

Tried it, but it looks like the settings are not reloaded.

@adamruzicka
Copy link
Contributor

Are you sure the code changes propagated to all the processes? It seems to work just fine for me

ekohl
ekohl reviewed Jun 9, 2021
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Here it states it's 2.4+ compatible. Would this also work on Foreman 2.4 or should the minimum version be raised?

foreman-tasks/lib/foreman_tasks/engine.rb

Line 36 in 86c7e83

requires_foreman '>= 2.4.0'

@ShimShtein ShimShtein mentioned this pull request Jun 9, 2021
Closed
@ShimShtein
Copy link
Member

@ekohl @ezr-ondrej, I have created another PR, since I couldn't change this one. I have modified the original PR so it would also work for ActiveJobs and modified the minimum requirement, since Foreman.settings is not available in older versions.

@ezr-ondrej ezr-ondrej force-pushed the reload_setting branch 2 times, most recently from b20ca1f to d6c7ede Compare June 9, 2021 12:10
@ezr-ondrej ezr-ondrej force-pushed the reload_setting branch 2 times, most recently from f905b40 to 8576481 Compare June 9, 2021 12:29
@adamruzicka
Copy link
Contributor

Test failures on older rubies seem to be related to kwarg handling changes

@ezr-ondrej
Copy link
Member Author

I believe kwarks are not needed, so I droped them. We should not temper with the action arguments IMHO and we do not have kwarks.

@ezr-ondrej
Fixes #32729 - reload setting prior run
78f168a 
In Foreman we've switched to in-memory setting cache, that needs to be
reloaded on demand, Rails is doing that on every request, for dynflow
actions we need to do it on every phase.

It does that only if the Foreman version is 2.5 or higher, as for older
versions this would fail.
@adamruzicka
Copy link
Contributor

@ezr-ondrej Forgot to push?

@ShimShtein @ezr-ondrej
Refs #32729 - Move middleware to engine
f681716 
To include it also in ActiveJob
@ezr-ondrej
Copy link
Member Author

Now done... I was moving it from commit to commit and didn't do it properly 🤦

@adamruzicka adamruzicka merged commit 962ca65 into theforeman:master Jun 10, 2021
@adamruzicka
Copy link
Contributor

Thank you everyone!

@ezr-ondrej ezr-ondrej deleted the reload_setting branch June 10, 2021 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl left review comments

@adamruzicka adamruzicka adamruzicka approved these changes

Assignees
No one assigned
Labels
Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ezr-ondrej @adamruzicka @ShimShtein @ekohl @theforeman-bot