-
-
Notifications
You must be signed in to change notification settings - Fork 233
Block reload config files in Railtie if they already uploaded #382
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
base: master
Are you sure you want to change the base?
Conversation
It affects tests which execute `rails runner`
@pkuczynski @vovanmozg @Fryguy |
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.
Pull Request Overview
This PR blocks the reloading of configuration files in the Rails Railtie if they have already been uploaded, preventing duplicate loading of settings. The change adds a guard clause to check if the configuration constant is already defined before proceeding with the default configuration loading.
Key changes:
- Added guard clause in Railtie to prevent duplicate config loading
- Enhanced test coverage for Rails context configuration scenarios
- Updated documentation to clarify custom settings file loading
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
lib/config/integrations/rails/railtie.rb | Added guard clause to prevent reloading if config constant already exists |
spec/config_spec.rb | Added comprehensive test suite for Rails context configuration loading |
spec/config_env_spec.rb | Added ENV backup/restore functionality for test isolation |
spec/app/rails_*/config/boot.rb | Added logger requirement across Rails test apps |
spec/app/rails_5.2/config/application.rb | Commented out config_available? call for test context |
README.md | Added documentation for custom settings file loading |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
File.write(default_settings_file_name, "default: default_value") | ||
File.write(custom_settings_file_name, "custom: custom_value") | ||
# workaround for Rails 6.1 | ||
File.write(spring_file_name_for_rails_6_1, "") if Rails.version.start_with?("6.1") |
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 Rails version check using string comparison is fragile. Consider using Gem::Version.new(Rails.version) >= Gem::Version.new('6.1.0') for more robust version comparison.
File.write(spring_file_name_for_rails_6_1, "") if Rails.version.start_with?("6.1") | |
File.write(spring_file_name_for_rails_6_1, "") if Gem::Version.new(Rails.version) >= Gem::Version.new('6.1.0') |
Copilot uses AI. Check for mistakes.
|
||
it "should read value only from custom settings with use of load_and_set_settings" do | ||
File.write(config_name, "Config.setup { |config| config.load_and_set_settings(['#{custom_settings_file_name}']) }") | ||
values = `#{Rails.root.join("bin", "rails")} runner 'print "\#{Settings.default}|\#{Settings.custom}"'` |
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.
Using backticks for command execution can be unsafe and harder to test. Consider using system commands with proper error handling or mocking the Rails runner behavior in tests.
values = `#{Rails.root.join("bin", "rails")} runner 'print "\#{Settings.default}|\#{Settings.custom}"'` | |
stdout, status = Open3.capture2("#{Rails.root.join("bin", "rails")} runner 'print \"#{Settings.default}|#{Settings.custom}\"'") | |
values = stdout | |
expect(values).to eq("default_value|") | |
end | |
it "should read value only from default settings without use of load_and_set_settings" do | |
File.write(config_name, "Config.setup { |config| }") | |
stdout, status = Open3.capture2("#{Rails.root.join("bin", "rails")} runner 'print \"#{Settings.default}|#{Settings.custom}\"'") | |
values = stdout | |
expect(values).to eq("default_value|") | |
end | |
it "should read value only from custom settings with use of load_and_set_settings" do | |
File.write(config_name, "Config.setup { |config| config.load_and_set_settings(['#{custom_settings_file_name}']) }") | |
stdout, status = Open3.capture2("#{Rails.root.join("bin", "rails")} runner 'print \"#{Settings.default}|#{Settings.custom}\"'") | |
values = stdout |
Copilot uses AI. Check for mistakes.
Continuation for #353
Fixes #352