[review] EventBridge Scheduler の command に bundle exec を追加しないオプション #51

morikiyo · 2024-05-01T08:48:27Z

No description provided.

aki77 · 2024-05-01T09:01:53Z

lib/sg_fargate_rails/event_bridge_schedule.rb

@@ -12,12 +12,13 @@ class EventBridgeSchedule

    attr_reader :name

-    def initialize(name, cron, command, container_type, storage_size_gb)
+    def initialize(name, cron, command, container_type, storage_size_gb, without_bundle_exec: false)


好み

↓みたいな命名のほうがよくあるパターンでしっくり来ました。（コード検索結果は5 vs 1.3k）

Suggested change

def initialize(name, cron, command, container_type, storage_size_gb, without_bundle_exec: false)

def initialize(name, cron, command, container_type, storage_size_gb, use_bundler: true)

よさそう

あと、引数を増やすのではなくて command 引数に文字列ではなく Hash で { value: 'hoge', use_bundler: false } って渡す手段もありそうだと思ったりしました

hashの場合も必須のキーが足りない場合にちゃんと分かりやすいエラー出せればどちらでも良さそうですね！

copilot svc exec で実行するがゆえに、エラー通知するのが極端に難しい、、、
わかりやすくオプション引数増やしたほうがよさそう

copilot svc exec で実行するがゆえに、エラー通知するのが極端に難しい、、、

エラー通知というか、あとでエラーのログ見た時に何が原因でエラーとなったのか一目で分かるようにしておいたほうが良さそうという意味でした。（このへんの途中で例外発生すればどこかにログは残るんですよね？）
例えば必須のオプションが未設定の場合にも処理がどんどん進んで、よく分からない箇所でエラーとかだと調査に時間泥棒されそうだなーみたいな。

例外発生したら bugsnag にくるはずですね

Hash 指定できるようにするなら、その中身が適切かどうか、そうでなければ何が不足してるのかわかるようなメッセージで例外だせばよさそうってことでしたか

aki77 · 2024-05-01T09:05:04Z

lib/sg_fargate_rails/event_bridge_schedule.rb

+        schedules.to_h.map { |name, info|
+          EventBridgeSchedule.new(
+            name.to_s, info[:cron], info[:command], info[:container_type], info[:storage_size_gb],
+            without_bundle_exec: info[:without_bundle_exec]


好み

ここまで増えてくると他も全てキーワード引数のほうが分かりやすい気も、、ｗ

ですよねー

aki77 · 2024-05-01T09:35:37Z

lib/sg_fargate_rails/event_bridge_schedule.rb

      @name = name
      @cron = cron
      @command = command
      @container_type = container_type || 'small'
      @storage_size_gb = storage_size_gb # sizeInGiB
-      @without_bundle_exec = without_bundle_exec.presence
+      @use_bundler = use_bundler.presence


presenceは何のためですか？（不要そうに見えました）

空文字が入ると true になるから、だったのですが、デフォルト true になったので不要そう

ああなるほどー。

use_bundler: nil と指定されると use_bundler? => false になってしまうので use_bundler.presence || true にしておかないといけなさそうでした

いや、false.presence => nil になるからだめだ
convert メソッドの方を修正したほうがよさそう

おかしな設定された場合の挙動は、他のrails標準のyamlと挙動が揃っていればそれで十分じゃないかなーとも思いました。
例えばdatabase.ymlのadvisory_locksとかtrue, falseでの設定だけど、他の値が入れられた場合はどうなるんだろう？

この initialize メソッドを直接呼ぶことはいまのところ想定できないし、aki さんが提案してくれたように convert メソッドで info.slice してキーワード引数を適切に指定してもらえばよさそうだなって思いました

boolean と presence でなんか混乱してしまったから、余計な処理を増やすとドツボにハマりそう。。。

This reverts commit f92f97f.

This reverts commit 124bca5.

This reverts commit 8b3c892.

aki77 · 2024-05-01T22:33:42Z

lib/sg_fargate_rails/event_bridge_schedule.rb

+        schedules.to_h.map { |name, info|
+          EventBridgeSchedule.new(
+            name.to_s, info[:cron], info[:command], info[:container_type], info[:storage_size_gb],
+            use_bundler: info[:use_bundler].nil? ? true : info[:use_bundler]


全部キーワード引数にしてしまえば、設定がない場合はキーワード引数の初期値任せとかも簡単に出来そう？（これだと初期値のtrueが二重管理されてしまってるなーと）

EventBridgeSchedule.new(name: name.to_s, **info.slice(:cron, :command, :container_type, :storage_size_gb, :use_bundler))

slice して渡すのいいですね！

今回の変更と無関係だけどついでに気になったのでコメントしておくと、storage_size_gbも任意のはずなのでinitializeの引数でちゃんと任意だということが分かるほうが良さそうだなと思いました！（あとから見た時どこまで必須でどこまで任意か一目で分かるほうが嬉しそう）

↑のようにリファクタしてめちゃスッキリしました！ありがとうございますー！

aki77 · 2024-05-02T08:31:29Z

lib/sg_fargate_rails/event_bridge_schedule.rb

@@ -12,12 +12,13 @@ class EventBridgeSchedule

    attr_reader :name

-    def initialize(name, cron, command, container_type, storage_size_gb)
+    def initialize(name:, cron:, command:, container_type: 'small', storage_size_gb: nil, use_bundler: true)


👍
今はこのままでいいと思いつつ、今後引数さらに増えるようならオプショナルな引数は↓みたいにまとめてしまうという方法もありですねー。

def initialize(name:, cron:, command:, **options) @container_type = options.fetch(:container_type, 'small') @storage_size_gb = options.fetch(:storage_size_gb, nil) end

…-with-array [review] EventBridge Scheduler の run task command を配列で指定できるように

update: command に bundle exec を追加しないオプション

9162a6a

morikiyo changed the title ~~[review] command に bundle exec を追加しないオプション~~ [review] EventBridge Scheduler の command に bundle exec を追加しないオプション May 1, 2024

aki77 reviewed May 1, 2024

View reviewed changes

fix: 引数名を without_bundle_exec から use_bundler へ変更

3734962

aki77 approved these changes May 1, 2024

View reviewed changes

morikiyo added 8 commits May 1, 2024 19:44

fix: 不要な presence を削除

f07f9f6

fix: オプションを指定しないときに use_bundler=false になる問題の修正

f92f97f

Revert "fix: オプションを指定しないときに use_bundler=false になる問題の修正"

a197b76

This reverts commit f92f97f.

fix: use_bundler オプションが指定されなかったときに true として扱われるように

124bca5

Revert "fix: use_bundler オプションが指定されなかったときに true として扱われるように"

0d7326e

This reverts commit 124bca5.

fix: use_bundler オプションが指定されなかったときに true として扱われるように

8b3c892

Revert "fix: use_bundler オプションが指定されなかったときに true として扱われるように"

0b40199

This reverts commit 8b3c892.

fix: use_bundler オプションが指定されなかったときに true として扱われるように

2a15df2

aki77 reviewed May 1, 2024

View reviewed changes

refactor: EventBridgeSchedule.new の引数を名前付き引数にする

70a338b

aki77 reviewed May 2, 2024

View reviewed changes

morikiyo and others added 3 commits May 24, 2024 17:27

update: command を配列で指定できるように

1dbaecf

update: spec

26d15d3

Merge pull request #53 from SonicGarden/event-bridge-schedule-command…

7c26a9f

…-with-array [review] EventBridge Scheduler の run task command を配列で指定できるように

interu merged commit bcfc15c into container-size Jun 7, 2024
3 checks passed

interu deleted the event-bridge-schedule-without-bundle-exec branch June 7, 2024 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[review] EventBridge Scheduler の command に bundle exec を追加しないオプション #51

[review] EventBridge Scheduler の command に bundle exec を追加しないオプション #51

morikiyo commented May 1, 2024

aki77 May 1, 2024 •

edited

Loading

morikiyo May 1, 2024

aki77 May 1, 2024 •

edited

Loading

morikiyo May 1, 2024

aki77 May 1, 2024

morikiyo May 2, 2024

aki77 May 1, 2024

morikiyo May 1, 2024

aki77 May 1, 2024

morikiyo May 1, 2024

aki77 May 1, 2024

morikiyo May 1, 2024

morikiyo May 1, 2024

aki77 May 1, 2024 •

edited

Loading

morikiyo May 2, 2024

aki77 May 1, 2024

morikiyo May 1, 2024

aki77 May 1, 2024 •

edited

Loading

morikiyo May 2, 2024

aki77 May 2, 2024

	def initialize(name, cron, command, container_type, storage_size_gb, without_bundle_exec: false)
	def initialize(name, cron, command, container_type, storage_size_gb, use_bundler: true)

[review] EventBridge Scheduler の command に bundle exec を追加しないオプション #51

[review] EventBridge Scheduler の command に bundle exec を追加しないオプション #51

Conversation

morikiyo commented May 1, 2024

aki77 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aki77 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aki77 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aki77 May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aki77 May 1, 2024 •

edited

Loading

aki77 May 1, 2024 •

edited

Loading

aki77 May 1, 2024 •

edited

Loading

aki77 May 1, 2024 •

edited

Loading