Repository navigation
global_nginx_config DSL を追加 - #72
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for global Nginx configurations (outside the http {} block) by updating the DSL evaluator, loader, and config context to accumulate and propagate these settings. It also updates the Nginx renderer and adds corresponding tests. The review feedback highlights three important issues: first, get_global_nginx_config returns an empty string instead of nil when empty, which causes the ERB template to render unnecessary empty lines; second, the unless configurations check in ReloadController does not handle empty arrays, potentially leading to a nil error when accessing configurations[0]; and third, assigning the same global_configs array reference to all contexts creates a shared mutable state that should be avoided by duplicating the array with .dup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| def get_global_nginx_config | ||
| @global_nginx_configs&.flatten&.join("\n") | ||
| end |
There was a problem hiding this comment.
@global_nginx_configs は initialize メソッドで [] に初期化されているため、設定が存在しない(空の)場合、get_global_nginx_config は空文字列 "" を返します。
Ruby では空文字列 "" は真(truthy)として評価されるため、nginx.conf.erb 内の <% if config&.get_global_nginx_config %> という条件分岐が真になり、不要な空行が出力されてしまいます。また、YARD コメントの @return [String] ...(存在しない場合は nil) という記述とも乖離しています。
配列が空の場合は nil を返すように修正することをおすすめします。
def get_global_nginx_config
return nil if @global_nginx_configs.nil? || @global_nginx_configs.empty?
@global_nginx_configs.flatten.join("\n")
endThere was a problem hiding this comment.
設定が存在しない場合 @global_nginx_configs.empty?が真を返すので nil がreturn されるので正しいかと思います。
| configurations = get_config | ||
| raise 'no configurations. please set $CONFIG_PATH or $PROXY_TO env var.' unless configurations |
There was a problem hiding this comment.
unless configurations によるチェックは、configurations が nil または false の場合のみ真となります。
もし設定ファイルが空などの理由で get_config が空配列 [] を返した場合、このチェックを通過してしまい、configurations[0] が nil になります。その結果、NginxMain::Renderer.new(nil) が呼び出され、予期しない挙動やエラーを引き起こす可能性があります。
配列が空の場合も含めてチェックするように、configurations.nil? || configurations.empty? を使用することをおすすめします。
configurations = get_config
raise 'no configurations. please set $CONFIG_PATH or $PROXY_TO env var.' if configurations.nil? || configurations.empty?| results.each do |config| | ||
| config.global_nginx_configs = global_configs | ||
| end |
There was a problem hiding this comment.
global_configs の同一の配列オブジェクトの参照を、すべての ConfigContext インスタンスの global_nginx_configs に代入しています。
これにより、ある ConfigContext インスタンスが global_nginx_configs 配列を破壊的に変更した場合、他のすべてのインスタンスの設定にも影響が及ぶ(共有ミュータブル状態)リスクがあります。
安全のため、各インスタンスに代入する際は dup を用いて配列を複製することをおすすめします。
results.each do |config|
config.global_nginx_configs = global_configs.dup
end
haruyan-hopemucci
left a comment
There was a problem hiding this comment.
レビューOKです ! ありがとうございました!
| def get_global_nginx_config | ||
| @global_nginx_configs&.flatten&.join("\n") | ||
| end |
There was a problem hiding this comment.
設定が存在しない場合 @global_nginx_configs.empty?が真を返すので nil がreturn されるので正しいかと思います。
| assert_equal 'so_keepalive=on deferred rcvbuf=8192', config.listen_options | ||
| end | ||
|
|
||
| def test_グローバル_nginx_configが複数回の呼び出しで蓄積されること |
There was a problem hiding this comment.
👍
テストの観点がわかりやすいです!
No description provided.