Review 36 imo #41
Review 36 imo #41
Conversation
- modelsに構造体を分けなくてもよさそうだったので統合 - パッケージ名を変更（これはdriverのままでもいい気はするけど、ラッパーかなと感じました） - 構造体名やファイル名を変更。gitとgithubで伝わるかなと思ったけどちょっと短すぎる気もする。
| @@ -161,6 +163,7 @@ func (r *ReviewAppReconciler) reconcileCheckAppRepository(ctx context.Context, r | |||
| return ctrl.Result{}, nil | |||
| } | |||
|
|
|||
| // comment: 何を行う関数なのか、関数名から読み取れませんでした | |||
There was a problem hiding this comment.
reconcile 対象の Reviewapp オブジェクトに紐づく At (ApplicationTemplate) , Mt (ManifestsTemplate) カスタムリソースのオブジェクトの更新を確認する関数です。
There was a problem hiding this comment.
省略せずにApplicationTemplateとManifestTemplateを使ってもいいかもしれないすね
| // comment: この関数内で呼んでいるreconcile~の関数名ですが、reconcileとCheck、reconcileとUpdateのように動詞が続いているのが気になりました。 | ||
| // reconcileCheckAppRepository なら reconcileAppRepository , reconcileUpdateInfraReposiotry なら reconcileInfraReposiotry という名前がよいように思います。 | ||
| // 追記: でもreconcileDeleteは割とよく使われてるしあんまり気にしなくてもいいかも |
There was a problem hiding this comment.
指摘されて初めて気づきました、たしかに reconcile (動詞) + check (動詞) は微妙だと感じます。
(cluster-api を参考実装としてるのですが、そこでも reconcileDelete 以外は reconcile 対象のリソース名がサフィックスに付いてそうです)
ちなみにここの reconcileXX 関数の呼び出しは Miro の ReviewApp コントローラの状態遷移 に対応しています。
Miro に書かれた状態遷移図に習って reconcileWatchingAppRepo だったり reconcileCheckedAppRepo という関数名にするのも案としてあるかと考えましたが、 「reconcile (調整する) + WatchingAppRepo (AppRepo を見ている)」は違うよなあとなっており、悩みどころです。
(Miro の状態遷移図の状態名が悪そうな気がしてきていますが自分の中で改善案が出てないです...)
There was a problem hiding this comment.
reconcileCheckAppRepository なら reconcileAppRepository , reconcileUpdateInfraReposiotry なら reconcileInfraReposiotry という名前がよいように思います。
上記に対する回答の前に、 Miro の ReviewApp コントローラの状態遷移 の図を文章で説明させてください。
今回自分が作った状態遷移図は 1 state = 1 resource のようになっておらず、以下の4つを state としそれらの間を遷移しています。
WatchingAppRepo: app-repo の PR の open/close の watch をする stateWatchingTemplate: k8s ApplicationTemplate, ManifestsTemplate オブジェクトの watch をする stateCheckedAppRepo:WatchingAppRepoまたはWatchingTemplateにて watch 対象に更新が会ったときにの遷移先 state。更新内容に沿った infra-repo へのマニフェストの push を実行するUpdatedInfraRepo:CheckedAppRepoの push が成功した後の遷移先 state。k8s cluster の ArgoCD Application オブジェクトを watch し更新があれば app-repo の該当 PR に comment を POST したあとWatchingAppRepoに遷移する。
ここで、現状の reviewapp コントローラの「1 state = 1 resource のようになっていない」という設計より、たとえば「AppRepo を reconcile する」ことは WatchingAppRepo state でも実施するし UpdatedInfraRepo state でも実施しているので、上記で示してもらっているような命名 (「reconcileCheckAppRepository なら reconcileAppRepository」) はコンフリクトしてるなーという違和感を持っています。
// そもそも状態遷移図の作り方が悪いのかなと思い始めているので、 Miro の図に対する指摘があると嬉しいです🙇♂️
There was a problem hiding this comment.
ちょっと遷移図含めて考えてみたんですが、こういう感じだと違和感ないかなと思いました。
- 自身のStatus以外に存在する変更を加える対象はInfraRepoとPR上のコメントの2つなので、reconcileをつける関数は
reconcileInfraRepoとreconcileCommentOfReviewAppURLだけ reconcileCheckAppRepositoryとreconcileCheckAtAndMtはどちらかというと InfraRepoとコメントを作成・更新すべきかどうかの判断を行っていると思うので、reconcileという関数名は使わない（とりあえずconfirmとしてみたけどなんとなくニュアンスが違う気もする)- 遷移的には、
CheckedAppRepoはNeedToUpdateInfraRepoのようにInfraRepoを更新する必要がある状態としたほうがわかりやすいかもしれない
if reflect.DeepEqual(ra.Status, dreamkastv1beta1.ReviewAppStatus{}) ||
ra.Status.Sync.Status == dreamkastv1beta1.SyncStatusCodeWatchingAppRepo {
// AppRepoが更新されているかどうかを確認する
result, err = r.confirmAppRepoIsUpdated(ctx, ra)
if err != nil {
errs = append(errs, err)
}
}
// ApplicationTemplateとManifestTemplateが更新されているかどうかを確認する
if ra.Status.Sync.Status == dreamkastv1beta1.SyncStatusCodeWatchingTemplates {
result, err = r.confirmTemplatesAreUpdated(ctx, ra)
if err != nil {
errs = append(errs, err)
}
}
// Infra Repoをspec.applicationとspec.manifestの内容に更新する
if ra.Status.Sync.Status == dreamkastv1beta1.SyncStatusCodeNeedToUpdateInfraRepo {
result, err = r.reconcileInfraReposiotry(ctx, ra)
if err != nil {
errs = append(errs, err)
}
}
// ReviewAppのURLをPRのコメントとして投稿・更新する
if ra.Status.Sync.Status == dreamkastv1beta1.SyncStatusCodeUpdatedInfraRepo {
result, err = r.reconcileCommentOfReviewAppURL(ctx, ra)
if err != nil {
errs = append(errs, err)
}
}There was a problem hiding this comment.
元々よりとても良くなってると感じますが、「confirm」も「infra-repo への commit」も「app-repo の PR へのコメントの POST」も全部引っくるめて reconcile だと思っていて、 confirm と reconcile が同じレベルにあるのは個人的にまだ違和感です...
上記処理自体が reconcile メソッド内の処理なので状態遷移する関数名には reconcile を使わない方針で、以下でどうでしょうか！
reconcileInfraReposiotry→commitToInfraReporeconcileCommentOfReviewAppURL→commentToAppRepoPullRequest- comment 内容は yaml で外から与える値なので
CommentOfReviewAppURLではないかなと思いました
- comment 内容は yaml で外から与える値なので
追記
いや、reconcileDelete も「infra-repo に対する commit」を実施するので commitToInfraRepo はふさわしくなさそうでした。 deployReviewAppManifestsToInfraRepo でどうでしょう？
| @@ -143,7 +143,7 @@ func (r *ReviewAppReconciler) reconcileCheckAppRepository(ctx context.Context, r | |||
| } | |||
|
|
|||
| // get ArgoCD Application name | |||
| argocdAppNamespacedName, err := kubernetes.PickNamespacedNameFromObjectStr(ctx, ra.Spec.Application) | |||
| argocdAppNamespacedName, err := kubernetes.GetNamespacedNameFromObjectStr(ctx, ra.Spec.Application) | |||
There was a problem hiding this comment.
- GetXXX は Kubernetes Cluster からリソースの GET を実行
- PickXXX はメソッドの引数に与えた string 型なオブジェクトから特定フィールドの値を抜き出す
という書き分けです。
|
基本的にこの変更後のほうが良いと思います！ 1 点だけ気になるところを挙げるならば、 というのも、gateways は "Clean Architecture の例の図" に出てくる名前として 「interface を通して Usecase (reviewapp-operator の場合 現状 gateways パッケージ以下のメソッドは services 以下からしか呼び出されておらず、今後機能拡張をした際もその制限を破りたくないと思っているため、上述したとおり gateways という名前を採用したいです。 ちなみに「どのパッケージからも呼び出されることを許容している」パッケージとして reviewapp-operator では utils というディレクトリを切っているので、もし wrapper という命名にするならば utils 以下に入れたいです。 |
|
#41 (comment) (9c8f8e7) の追記です。
結局自分が達成したいことは ↑ なので、それさえ認識が合えば wrapper という名前でもいいかと思って来ました！ (ただ現状の自分の知識に基づくとベストは gateways かなーと思ってます、こっちのほうが良いみたいな意見があればいただけると嬉しいです:pray:) それと、 |
|
パッケージ名は自分ならこうするかな、くらいなので強い意見はないです。なのでgatewayでも特に問題はないです。どちらかというとinterface1個しかないのにパッケージを分けるような細かいパッケージ分割がGoっぽくないかなと感じています。 パッケージ分割まわりはIMOなので、反映するかはおまかせします〜。 |
IMO, nits, refactoring系などコアロジックとは関係ない場所で気になったものをまずはリストアップしています。
#36