Repository navigation
設定ファイルを書き出す責務をクラス分割 - #65
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the configuration generation logic by extracting Nginx and logrotate rendering into dedicated renderer classes and adding corresponding unit tests. Feedback highlights critical issues where Ruby 3.1 keyword argument shorthand is incorrectly applied to methods, which will result in NameErrors. Additionally, a new check for the 'socat' binary in the SocatManager initializer is flagged as out-of-scope and potentially disruptive to environments not requiring socat.
| if @no_ssl | ||
| # HTTPS 非対応 | ||
| render_nginx template: :http | ||
| nginx_renderer.render(template: :http, output_path:) |
There was a problem hiding this comment.
Ruby 3.1 で導入されたキーワード引数の省略記法 (key:) は、ローカル変数に対してのみ有効です。output_path はメソッドであるため、この記法では NameError が発生します。メソッドの結果を渡す場合は、明示的に output_path: output_path と記述する必要があります。
nginx_renderer.render(template: :http, output_path: output_path)| # 証明書ファイルが指定されていなければ、Let's Encrypt を使って生成する | ||
| if force_update_cert || !File.exist?("/etc/letsencrypt/live/#{domain}") | ||
| render_nginx template: :cert | ||
| nginx_renderer.render(template: :cert, output_path:) |
There was a problem hiding this comment.
前述の通り、output_path はメソッドであるため、キーワード引数の省略記法は使用できません。明示的に output_path: output_path と記述してください。
nginx_renderer.render(template: :cert, output_path: output_path)| end | ||
|
|
||
| render_nginx template: :https | ||
| nginx_renderer.render(template: :https, output_path:) |
There was a problem hiding this comment.
前述の通り、output_path はメソッドであるため、キーワード引数の省略記法は使用できません。明示的に output_path: output_path と記述してください。
nginx_renderer.render(template: :https, output_path: output_path)|
|
||
| def initialize | ||
| @dest_list = {} # { dest => unix_socket } な連想配列 | ||
| raise "socat is not installed" unless system('which socat > /dev/null 2>&1') |
There was a problem hiding this comment.
この変更は、設定ファイルの書き出し責務の分割という本 PR の主旨とは無関係な変更（サイドエフェクト）に見えます。また、SocatManager は Singleton であり、initialize で例外を投げると、socat を使用しない設定（アダプターが direct の場合など）であっても SocatManager.instance が呼ばれた時点でアプリケーションがクラッシュしてしまいます。このチェックは、実際に socat を必要とする処理の直前で行うか、あるいは設定を確認して socat が必要な場合にのみ実行されるようにすべきです。
| require_relative 'test_helper' | ||
|
|
||
| Dir[File.join(__dir__, "*_test.rb")].sort.each do |file| | ||
| Dir[File.join(__dir__, "**/*_test.rb")].sort.each do |file| |
|
|
||
| def initialize | ||
| @dest_list = {} # { dest => unix_socket } な連想配列 | ||
| raise "socat is not installed" unless system('which socat > /dev/null 2>&1') |
There was a problem hiding this comment.
socat がインストールされているかのチェックを追加
haruyan-hopemucci
left a comment
haruyan-hopemucci
left a comment
There was a problem hiding this comment.
レビューOKです! ありがとうございました!
| class BaseRenderer | ||
| def initialize(config) | ||
| @config = config | ||
| end | ||
|
|
||
| def render(template_path:, output_path:) |
| def render(template:, output_path:) | ||
| template_path = File.join(TEMPLATE_DIR, "#{template}.erb") | ||
| super template_path:, output_path: | ||
| end |
| if @no_ssl | ||
| # HTTPS 非対応 | ||
| render_nginx template: :http | ||
| nginx_renderer.render(template: :http, output_path:) |
| require 'lib/config' | ||
| require 'lib/renderers/nginx_by_domain/renderer' | ||
|
|
||
| class NginxByDomainRendererTest < Minitest::Test |
There was a problem hiding this comment.
👍
レンダラーごとのテストに分割できて保守性が上がったと思います!
2 participants
#57 (comment)
Config#generate_nginx_configがまだ残っていますが、これも将来的にはクラス切り出ししていく流れです。