旧ToyboxへのプロキシAPIの追加#123
Conversation
simesaba80
left a comment
simesaba80
left a comment
There was a problem hiding this comment.
レビューしました、該当箇所の変更をお願いします
またREADME.mdに書いてあるコミットの規則が守られてないので注意してください
ブランチ名についても今回は修正しなくていいですがissue番号の#は不要です
また閲覧したかはわかりませんがdocs/deesign-doc.mdを参照すると記述場所が適切に決めやすいと思います
あと既に一部されてない部分がありますがgofmtによるフォーマッタはかけてください
今回のPRでプロジェクト全体にgofmtをかけてほしいです
| return r.echo | ||
| } | ||
|
|
||
| func newLegacyToyBoxProxy(upstreamBaseURL string) echo.HandlerFunc { |
There was a problem hiding this comment.
routerはあくまでルーティング関連の設定をする場所なのでこの関数がrouter.goにあるのはdesign-doc.mdに書いている設計の意図を外れてる気がします
toybox-back/docs/design-doc.md
Lines 257 to 265 in e084f0e
現在書いてる用途的には少し外れてるけど外部との通信だからinfrastructure/external配下が適切じゃないかな
externalの用途少し拡張することになるのでdesign-docの変更まで含めて関数の場所を引っ越してください
| @@ -0,0 +1,72 @@ | |||
| package router | |||
There was a problem hiding this comment.
テストに関しても同様にexternalに引っ越して
| } | ||
| } | ||
|
|
||
| proxy := httputil.NewSingleHostReverseProxy(targetURL) |
There was a problem hiding this comment.
ここに関しては言ってなかったのが悪いんだけどhttpリクエストのHostは環境変数で上書きできるようにしてほしい
| } | ||
| } | ||
|
|
||
| proxy := httputil.NewSingleHostReverseProxy(targetURL) |
There was a problem hiding this comment.
今回プロキシしたいToyBoxのAPIは全部認証のいらないAPIだね
ただAuthorizationヘッダーが付与されてる可能性があるのでここは明示的に削除しといてほしい
同じ理由でCookieも明示的に削除してほしいかな
2 participants
旧ToyboxへのProxyAPIを実装しました