コードレビューとPull Request、そしてその承認機能の副作用について考える - 時計を壊せ

用語 レビュアー 対象となるコードをレビューする人のことを指します。 レビュイー レビューを受ける人、つまりレビューする対象のコードを書いた人のことを指します。 tl;dr アプリケーション開発業務におけるコードレビューはコードの正しさや質そして一貫性を保ち、それらと同時にコードに対するチームとしての共有知を作り上げる良いプラクティスだと思います アプリケーション開発チーム内でのコードレビューにおいてPull Requestを使ったレビューのスタイルは一般的ですが、Pull Requestの承認は実際にはほとんど意味がないのではないでしょうか？ ほとんど意味がないにも関わらず、承認の有無によって業務フローが左右されることでそれが権威的に扱われてしまいオーナーシップを希薄化させ、結果的にコードレビューのコストが増加したりそれを行う目的を見失ってしまっていることはないでしょうか？ Pull R

[kagerou_ts]

特定の誰かにせず、メンバー全員がレビュアーで「○○人中○人」って形式にすれば権威化も薄れたりしないか。経験の浅い人も深い人もそこに混ぜる

[gfx]

全社的にapprove必須だけど問題はないな。そもそもコンプラ上誰もレビューしてないコードをデプロイするわけにはいかないし。レビューしてもPR主のオーナーシップは変わりませんよ、でいい気も。

[hitode909]

あくまでLooks Good To Meであって、私にとっては良いと思います、第三者から見ても大丈夫そう、くらいのニュアンスでapproveしてました

[kazkaz03]

挙げられている副次的な効果が発生した事ないためか、よくわからなかった。/レビュアーの指摘を鵜呑みにして問題が発生した例は、レビュアーの権威の問題なのか？

[ipa5963]

リング型承認と確認内容の見える化が必要。仕組みだけで質を担保しようとするのは甘え。チームに合わせて永遠に最適化を繰り返すだけ。

[nunulk]

approve の訳語は"賛成する"のほうがよかったのではないか、という気もする

[onk]

通らば PR、と手抜きしつつ出す場合はあって (オーナーシップの希薄化)、書いてあるようにチームに負担を強いている行為だけど、速のバランスが取れるなら今後もやっていく。とにかくレビューは関係性なんだよなー。

[ktoktokto]

「○○人中○人の承認」方式でレビュアーをアサインした場合、経験の浅いレビュアーは積極的にレビューしないことがほとんど。権威化を防ぐという目的には寄与しないかなぁ。

[dorapon2000]

“自分が考える理想的な状況は、メンバーの誰もが健全にオーナーシップを持って開発を行い、マージされても品質を毀損しない自信があるPull Requestだけが作成されることです”

[karikari1255]

ISMSとか上場後に承認フローどうなってるかとか言われちゃうので別で管理するのも嫌だからプルリクの承認機能使っているんだと思う。あれ嬉々としてやっている人もグループもいないというのはその通りだと思うけど

[ledsun]

「コードのオーナーシップの希薄化」って悪いことだったのか…「コードの共同所有」が進んでていいことだと思ってた。

[iekusup]

ほー。

[ryan_aircloset]

そこまで固くならなくてもと思うところもありつつ、レビュイーもオーナーシップを持つべきという意見には全面同意。

[TMCNE]

環境と人の性質にかなり依る問題なので、懸念している事が実際に起こった例がないと何も話しようが無くないか

[uminomezumi]

PR作るときに自分自身(レビューイ)assignするの感覚的にいいなと思って採用してたんだけど、コードへのオーナーシップとして説明すればいいのか、なるほど

[inabajunmr]

GitHubのapproveボタンのテキストが変わるだけでオーナーシップが増すかというと、あまり変わらないような気はする

[ssig33]

職場の雰囲気悪すぎでは、、、

[stefafafan]

まあでも実装したものがバグってた時にレビュー通ってたら、自分一人の責任ではなくチームの責任ということにしやすいのはあると思う…承認ってラベルをLGTMに変更すると良いのかな

[tg30yen]

レビューの指摘の重要度で区分けすれば緩和されると思う。must/proposalをわけてproposalに対応するかはレビュイーに任せる。mustに関してはレビュアーが責任を持つ。

[inamem9999]

承認ボタンをヨシ！ボタンにすればそれはそれで良くないか

[wata88]

looks good to meでしかないのでオーナーシップについては問題意識は無いかなぁ。そもそもdiffだけでレビューできることって限られててそこがツラい

[toshiwo]

捉え方の問題じゃないかなぁ「オーナーシップの希薄化」も逆から見れば実装者の手を離れても問題ないコード(と言えるまで実装とレビューをやりきる)ということでもあるし

[katzchang]

責任の希薄化はどちらの立場にもある。コードレビューで不具合を発見するのって実際難しいんだよね。

[chiroruxx]

あとでよむ

[koogawa]

“レビュアーの権威化” これ、とてもわかるんだよな

[heppokopg2013]

approveが必須だとオーナーシップが薄れるっていうのは他責思考すぎないかな？他の業務で考えた時、稟議が必要になったら自分の仕事じゃなくなるとか言わないし、ノーチェックでバンバン実行されるとか怖くない？

[june29]

Approve ナシでばんばんマージした方がうまくいくならそのチームはそれでいいと思う。自分が関わる場では「権威」や「承認」といった雰囲気を感じることはほぼないかなあ。

[vndn]

未承認マージを認めた場合、レビューと切り戻しを速やかに行う必要が出る上、変更箇所が重なった場合のタルさがパないのでは。マージ後レビュー前PRの管理といったツール面も気になる。

[umai_bow]

“承認の有無によって業務フローが左右されることでそれが権威的に扱われてしまいオーナーシップを希薄化” これらある！

[revert]

経験不足なメンバーで構成されていた所以の不幸かな。今回で学ぶだろうし強いメンターを用意できないなら多少の失敗は必要コストだよ。DevOpsな運用をしているならPRを安全側に倒すようになるから心配しなくていい

[uva]

しかしノーレビューでマージする副作用の方が大きそうだ

[teramako]

レビューアーに権威が寄るの分かる。