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

Fix config load for liveness probe #355

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Fix config load for liveness probe #355

merged 1 commit into from
Oct 4, 2023

Conversation

deepredsky
Copy link
Contributor

Some rails apps might chose to not use the racecal.yml file. Let's load known
config files if they are present similar to how cli handles them

@deepredsky deepredsky marked this pull request as draft September 29, 2023 09:22
@@ -36,10 +36,14 @@ def liveness_probe(args)
require "racecar/liveness_probe"
parse_options!(args)

if ENV["RAILS_ENV"]
if ENV["RAILS_ENV"] && File.exist?("config/racecar.yml")
Racecar.config.load_file("config/racecar.yml", ENV["RAILS_ENV"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should use this method for loading the config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dasch I've updated it to behave exactly as the cli load them

Choose a reason for hiding this comment

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

I am wondering if loading the config like this (RailsConfigFileLoader) is the right thing here, doesn't that mean the liveness probe will always try to boot the entire Rails app just to check the modified time of a file?
There is the without_rails config option, but what if the consumer needs Rails, then the liveness probe will do all that extra work for nothing.

Some rails apps might chose to not use the racecal.yml file. Let's load known
config files if they are present similar to how cli handles them
@deepredsky deepredsky marked this pull request as ready for review September 29, 2023 12:33
@deepredsky deepredsky requested a review from dasch September 29, 2023 12:33
@dasch dasch merged commit 5bcd34a into master Oct 4, 2023
6 checks passed
@dasch dasch deleted the liveness-probe branch October 4, 2023 08:24
@dasch
Copy link
Contributor

dasch commented Oct 4, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants