release: trigger release processes when release workflow succeeded #47

otegami · 2024-12-12T04:18:13Z

GitHub: GH-43

In this PR, we set up how release flow is triggered from webhook
requests.

otegami · 2024-12-19T06:12:36Z

I'm thinking I will add the test at the following PRs because this PR has already had a lots of changes.

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

ansible/files/home/deployer/webhook/lib/deployer/app.rb

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

ansible/files/home/deployer/webhook/lib/deployer/app.rb

abetomo · 2024-12-19T23:48:45Z

ansible/files/home/deployer/webhook/lib/deployer/app.rb

+
+      payload = parse_payload(request.body.read, response)
+      if payload.nil?
+        response.set(:bad_request, "invalid JSON format: <#{$!.message}>")


$! はここでも使えるんですか？

ここでは使えないので直してあげる必要がありそうです 🙏🏾

例外を上げるようにしたので、$!は使わないようになりました。

abetomo · 2024-12-20T00:38:24Z

ansible/files/home/deployer/webhook/lib/deployer/app.rb

      unless valid_signature?(request)
        response.set(:unauthorized, "Authorization failed")
        return
      end
+
+      payload = parse_payload(request.body.read, response)


parse_payload ってペイロードをパースする、という意味で合ってますか？
ペイロードをパースして出来上がるものは、payload ではない気がしたので、payload に代入しているのが気になりました。

bodyをparseするので、parse_bodyにしました。

komainu8 · 2024-12-20T03:35:51Z

一通り頂いた指摘に対応しました。

ansible/files/home/deployer/webhook/lib/deployer/app.rb

abetomo · 2024-12-20T08:55:54Z

ansible/files/home/deployer/webhook/lib/deployer/app.rb

+      release_tasks = Proc.new do
+        # TODO: call rake tasks for sign packages.
+      end
+      response.set_finish_proc(release_tasks)


引数で response をもらわないとだめっぽい。

デプロイ処理の結果をレスポンスとしては返せないよ。
どのくらい時間がかかるかわからないから。
こいつが返せるのはデプロイ処理を開始できたかどうかまでだよ。
デプロイ処理のエラーはレスポンスじゃなくてログに吐くとかして確認しないとダメだよ。
（あと、エラー情報に秘匿情報が含まれたらイヤだからそういう観点でもレスポンスとして返したくない。）

fix: 0be9ff2, 2f40dc8
レスポンス自体はすぐに返す想定です。
リリースに必要な作業は、スレッドを利用して非同期で対応しようと思います。
リリース作業で実行するRakeタスクに関するエラーに関しては、ログファイルに出力する予定です。

デプロイ処理の結果をレスポンスに使わないならset_finish_procしなくていいんじゃないかな。

たしかに必要ないですね。そのまま普通にThreadで動かしてあげればよいだけですね。

abetomo · 2024-12-20T09:00:12Z

ansible/files/home/deployer/webhook/lib/deployer/app.rb

+      end
+    end
+
+    def process_release(payload)


メソッド名は改善できそう。kick_release_task とかを考えましたが、それも違うか…。
process_release ではない感じがします。

deployにしました。

kou · 2024-12-20T09:13:08Z

ansible/files/home/deployer/webhook/lib/deployer/app.rb

      end
    end

-    def valid_signature?(request)
+    def valid_signature!(request)


例外を上げるようにするならvalidじゃなくてverifyとかじゃないかな。

verify_signature!にしました。

kou · 2024-12-20T09:15:53Z

ansible/files/home/deployer/webhook/lib/deployer/app.rb

+        payload = parse_body(request, response)
+        process_payload(payload)
+      rescue => e
+        response.set(:bad_request, e.message)


全部:bad_requestは乱暴じゃない？

raise RequestError.new(:unauthorized, "Authorization failure")とかするようにして、エラーメッセージだけじゃなくステータスも渡せるようにしたら？

RequestErrorじゃない例外があがったら:internal_server_errorとe.messageでいいと思うけど。

fix: 1481c70 予期できるエラーに関しては、RequestErrorでハンドリングするようにして、予期しないものに関してはサーバー側のエラーとして統一して返すようにしました。

kou · 2024-12-20T09:19:43Z

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

+
+    def workflow_tag?
+      return false unless branch
+      branch.match?(/^v\d+(\.\d+){2}$/)


Mroongaで動かなそう。
（MroongaはXX.YY。）

fix: a39c792 vX.Y.Z or vX.Yの形式だけにマッチするように修正してみました。

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

kou · 2024-12-20T09:22:33Z

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

+    def [](key)
+      key.split(".").inject(@data) do |current_data, current_key|
+        if current_data
+          current_data[current_key]
+        else
+          nil
+        end
+      end
+    end


最近のRubyにはHash#digがあるから、今ならこれを作らなくてもいいかもね。

あるいは、@data.dig(*key.split("."))にするか。

fix: 15cd16d Hash#digを利用したほうが、シンプルにかけるので修正しました。

kou · 2024-12-20T09:27:37Z

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

+
+module Deployer
+  class Payload
+    RELEASE_WORKFLOWS= ["Package"].map(&:freeze).freeze


Suggested change

RELEASE_WORKFLOWS= ["Package"].map(&:freeze).freeze

RELEASE_WORKFLOWS = ["Package"].map(&:freeze).freeze

そんなにfreezeしなくてもいい気はするけど。。。
Rubyを使っているのにそんなに信じられない？というか。。。

fix: be34927 そんなにfreezeしなくても良いと思うので、修正しました。

otegami · 2024-12-23T01:55:31Z

上からレビュー対応をしていきます。

GitHub: groongaGH-43 In this PR, we set up how release flow is triggered from webhook requests.

format

abetomo · 2024-12-23T03:17:58Z

ansible/files/home/deployer/webhook/lib/deployer/error.rb

+  class Error < StandardError
+  end
+
+  class RequestError < Error


よくわかってなくて初歩的な質問ですみません。
Error をはさむときと RequestError < StandardError するときとどういう違いがあるのですか？

Rubyは継承できるクラスが一つなので、今後拡張しやすいようにErrorのベースクラスは別で置いて置きたくて、このようにしてみました。（ただ、今回はまだ拡張予定が直近ではないのでなくても問題ないです。）

abetomo · 2024-12-23T03:24:06Z

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

+
+module Deployer
+  class Payload
+    RELEASE_WORKFLOWS= ["Package"].freeze


workflow_tag? はMroongaに対応してましたが、RELEASE_WORKFLOWS もMroongaを考慮する必要あり？

あぁ、Mroongaの方は"workflow_run"じゃなくて"release"のwebhookでやるので気にしなくてよくなりました。

https://github.com/mroonga/mroonga/blob/main/.github/workflows/release.yml
Mroongaの方はgh run watchで待って、gr release createで一気に全部アップロードするようにしてあります。

Because we can just use Thread to call release tasks.

otegami · 2024-12-23T03:59:19Z

ここまで頂いたレビューコメントの対応を行いました 🙏🏾

abetomo · 2024-12-23T04:07:59Z

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

+
+module Deployer
+  class Payload
+    RELEASE_WORKFLOWS= ["Package"].freeze


Windows用バイナリでも実行したいから CMake も追加するとよさげ？

必要ですね！追加します。

fix: e0b7514

kou · 2024-12-23T05:20:33Z

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

+
+module Deployer
+  class Payload
+    RELEASE_WORKFLOWS= ["Package"].freeze


Suggested change

RELEASE_WORKFLOWS= ["Package"].freeze

RELEASE_WORKFLOWS = ["Package"].freeze

fix: d94d95d

kou · 2024-12-23T05:21:22Z

ansible/files/home/deployer/webhook/lib/deployer/payload.rb

+
+    def workflow_tag?
+      return false unless branch
+      branch.match?(/^v\d+(\.\d+){1,2}$/)


Suggested change

branch.match?(/^v\d+(\.\d+){1,2}$/)

branch.match?(/\Av\d+(\.\d+){1,2}\z/)

https://www.clear-code.com/blog/2015/3/25.html

fix: bceac4f

break

otegami · 2024-12-23T05:40:18Z

ここまで頂いたレビューコメントの対応を行いました 🙏🏾

otegami force-pushed the release-run-async-task branch 3 times, most recently from a82b318 to e4f8727 Compare December 19, 2024 02:58

otegami changed the title ~~draf relase: run asyn task~~ release: trigger release processes when release workflow succeeded Dec 19, 2024

otegami marked this pull request as ready for review December 19, 2024 05:57

abetomo reviewed Dec 19, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/payload.rb Outdated Show resolved Hide resolved

abetomo reviewed Dec 19, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/app.rb Outdated Show resolved Hide resolved

abetomo reviewed Dec 19, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/app.rb Outdated Show resolved Hide resolved

komainu8 reviewed Dec 19, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/payload.rb Outdated Show resolved Hide resolved

komainu8 reviewed Dec 19, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/app.rb Outdated Show resolved Hide resolved

otegami force-pushed the release-run-async-task branch from e4f8727 to ff9fcc0 Compare December 19, 2024 08:27

abetomo reviewed Dec 19, 2024

View reviewed changes

abetomo reviewed Dec 20, 2024

View reviewed changes

komainu8 force-pushed the release-run-async-task branch from 5cae964 to c1d9868 Compare December 20, 2024 03:25

abetomo reviewed Dec 20, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/app.rb Outdated Show resolved Hide resolved

ansible/files/home/deployer/webhook/lib/deployer/app.rb Outdated Show resolved Hide resolved

abetomo reviewed Dec 20, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/app.rb Show resolved Hide resolved

abetomo reviewed Dec 20, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/app.rb Outdated Show resolved Hide resolved

abetomo reviewed Dec 20, 2024

View reviewed changes

kou reviewed Dec 20, 2024

View reviewed changes

ansible/files/home/deployer/webhook/lib/deployer/payload.rb Outdated Show resolved Hide resolved

kou reviewed Dec 20, 2024

View reviewed changes

otegami and others added 3 commits December 23, 2024 12:03

release: trigger release processes when release workflow succeeded

939b99e

GitHub: groongaGH-43 In this PR, we set up how release flow is triggered from webhook requests.

skip executing release tasks

61d9a1b

Simplify

999fb07

komainu8 and others added 11 commits December 23, 2024 12:03

Add a necessary argument

07b433e

Use more suitable name

bc59737

Use more suitable name

4a6812a

Fix wrong name

a548d2e

Fix wrong name

469ad67

Raise error status

764ce39

Remove unnecessary freeze

be34927

refactor Deploy::Payload#[] using Hash#dig

15cd16d

Simplify the logic using dig method

f3eecfd

Fix the condition of checking the tag because Mroonga's tag is like vX.Y

a39c792

format

add Deploer::RequestError class to handle the webhook errors

1481c70

otegami force-pushed the release-run-async-task branch from 774dee2 to 1481c70 Compare December 23, 2024 03:03

add ! because it might raise exceptions

b48457a

abetomo reviewed Dec 23, 2024

View reviewed changes

otegami added 2 commits December 23, 2024 12:19

call asynchronous release tasks

0be9ff2

add todo comments which we will deal with in the following PRs

2f40dc8

abetomo reviewed Dec 23, 2024

View reviewed changes

otegami force-pushed the release-run-async-task branch from 907e74f to 45a44ab Compare December 23, 2024 03:50

Remove unnecessary Deployer::Response#set_finish_proc

89dc83f

Because we can just use Thread to call release tasks.

otegami force-pushed the release-run-async-task branch from 45a44ab to 89dc83f Compare December 23, 2024 03:50

abetomo reviewed Dec 23, 2024

View reviewed changes

kou reviewed Dec 23, 2024

View reviewed changes

otegami added 3 commits December 23, 2024 14:24

add CMake workflow to the release targets for Windows packages

e0b7514

fix formats

d94d95d

fix regular expressions because we will just use the string without line

bceac4f

break

abetomo merged commit 491c627 into groonga:main Dec 23, 2024

otegami deleted the release-run-async-task branch December 23, 2024 05:47

	RELEASE_WORKFLOWS= ["Package"].map(&:freeze).freeze
	RELEASE_WORKFLOWS = ["Package"].map(&:freeze).freeze

	RELEASE_WORKFLOWS= ["Package"].freeze
	RELEASE_WORKFLOWS = ["Package"].freeze

	branch.match?(/^v\d+(\.\d+){1,2}$/)
	branch.match?(/\Av\d+(\.\d+){1,2}\z/)

release: trigger release processes when release workflow succeeded #47

release: trigger release processes when release workflow succeeded #47

Conversation

otegami commented Dec 12, 2024 • edited Loading

otegami commented Dec 19, 2024

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

komainu8 commented Dec 20, 2024

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

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

otegami commented Dec 23, 2024

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

otegami commented Dec 23, 2024

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

otegami commented Dec 23, 2024

otegami commented Dec 12, 2024 •

edited

Loading