PRのつくり方を考える

ここ数年くらいPRのつくり方について、尊敬する先輩に教えて頂いたりと色々試行錯誤があったので折角なので纏めていく。

TL;DR

  • big commitは避ける。1story /taskに対してPRは複数用意していい。とにかくbig commitは避ける。
  • review結果は反映するものではなく取り込むもの。
  • Testは頑張って書く。ひたすら頑張って書く。良いコードを一発で書けないからこそ頑張って書く。
  • コード解析やフォーマッティング等、気付きを自動化できる仕組みは積極的に使う。楽をして、浮いた時間は別なことに使う。

Pull Requestの処理にかかるカロリーを考える

（reviewee）Pull Requestを出す
（reviewer）観測する
（reviewer）内容を把握する
（reviewer）指摘を書く
（reviewee）Responseを書く or 修正のコミットを積む

のラリーを行うため、非常にカロリーが必要な業であることをまず理解する。 また、責任分散の目的を考えると2approve以上が望ましいため、reviewerのカロリーを下げることが非常に大事。

big commitをなるべく避ける

big commitというかbig pull requestというか。。。
change setをなるべく小さく、小口でPRを作成することが大事。

なぜbig commitを避けるのか

big commitは

  • review業がそもそも大変になる
  • 大変になるとどうしても見落としが出てくる
  • 大変なので後に回されやすくなる

といった感じになると実感している。

1Story / Task に対して 1PRである必要は全然ない

１PRは1Storyじゃなくて良い
という気付きを得てから、改めて理解したが、人間が気を付けられる量なんてたかが知れている。

例えば50ファイル、3000Lineとかchange setを出されたところで、正直見ようと思えなかったりする。（仕事だから見る努力はするけど、実際大味なレビューになるんですよね。）

なので、

  • コードベースを良くしていきたいのであれば、PRはどんどん分けたほうがいい。
  • 一方で、PRをチェインすると、最初のほうのPRで指摘があるとrebase業が死ぬほどダルくなることがある。
  • この辺は、git力を少し上げることで改善が可能なので、少しずつ頑張る。

みたいな感じで進めていくのが筋が良いように思う。
特に、git力については避けて通れない。もしチーム内でgit力が足りていないと感じたらラーニングしていくと運用が改善していくのでオススメ。

review結果は反映するものではなく取り込むもの

レビュー結果反映とか指摘結果反映とかレビュー指摘事項修正みたいなcommit comment
は、

  • 対応内容が明示されていないことが問題
  • レビュー結果について納得せずに、「はいはい、指摘されたから直しましたよ、これでいいんでしょ。」っていう感じになるのが問題

の２つの意味で筋が悪い。 特に後者の気持ちでは、人は育たないので気を付けたほうが良い。指摘事項反映（詳細な文章）みたいなのも、後者の問題に引っ掛かるので筋が悪い。
レビューの指摘結果は意図も含めて取り込むものであるという前提を忘れないようにしたい。

Testを頑張って書く

品質を語るならまずTestを書け。は本当にその通りで、

  • PRを出すときにはゴールデンパスのTestを頑張って書こう。
  • 業務アプリケーションの場合は永続化層込のTestを頑張って書こう。

が本当に大事。 reviewでもtestがなければ超緊急時を除いてapproveしないほうが筋が良い。

UNDONE: Testの効率とかは、まだまだ考えられてない

今の時代、Docker上での自動Testが主流なのだから、今のところ雰囲気で使ってるDockerをもっと理解することで改善が進むんだと思う。

dependabotはいいぞ

dependabotは、自動Testがいてくれることで、nuget packageのアップデート時のリスクがある程度何もしなくても可視化されてく感じがして素晴らしいと感じている。
一方で、どうしてもreleaseまでのgit のbranch modelを考えると、mainへのPR作成は、良し悪しというか、チームのパワーには若干合ってないとも感じるので、実際は、自分でPR作りなおすこともままある。（これはたぶん私が雰囲気で運用しているからというのもあると思う。）

github actionsでも、dotnet formatをかけて差分をPRにするworkflowを動かすようにしているが、結局のところコードベースがヘルシーな状態かどうかのインジケーターなんて多くて困ることはないからどんどん頑張ればいいんだと思う。

EF Core 6 のリバースエンジニアリングがNull許容参照型に対応したのが嬉しい話

EF Core6 は.NET6実装

EF Core5は.NET Standard 2.1での実装だったので、.NET Core 3.1以降であれば利用が可能でした。
対してEF Core 6は.NET 6での実装なので.NET 6での利用に限定されます。 これは、.NET Core 3.1が2022年11月、.NET5が2022年2月?でEOLになるからそらSupportしないよねという感想。

EF Core5のリバースエンジニアリングはNull許容参照型は未対応だった

EF Core5は制約事項として、リバースエンジニアリングでのEDM＆DBContext生成時Null許容参照型はdisableでした。
この辺結構嫌というか、令和の言語はnull安全であってほしい（基本的にはフロー解析ではじきたい）ので、enableになればいいなーと2年くらい思っていました。
※ 誤解のないように書くと、EF Core5はすごく出来の良いORMだったし、performanceもずいぶん改善されて来たので、1joinくらいならEF Coreでいいかなーというお気持ちでした。（CommandはEF Core、QueryはDapper派）

EF Core6はリバースエンジニアリングでもNull許容参照型に対応した

なので、今回嬉しいポイントはここでした。
csprojに<nullable>enable</nullable>を書かなきゃいけないですが、そんなものいくらでも書きます。
というか、vs2022になって、csprojを直接書くだけじゃなく、GUIを使ってもいいかと思えるようになるくらい、vs2022はUIが良変更ですしね。

NU1605を対処する

.NET C#

NU1605がBuild時に上手くいかないケースがある

自分が遭遇した現象

  • ローカルでのBuild（VisualStudio2022）ではBuildはPass。
  • ローカルでのBuild（dotnet build）ではBuildはPass。
  • Cloud BuildでのCI時のBuildはPass
  • Cloud Runへのdeploy時のBuildはError

ダウングレード問題

docs.microsoft.com

どうも、依存関係チェーンの関係で起きることがある。ということらしい。 Microsoft.NETCore.Targetsを追加することで解決する。

csprojに追加するもの

<PackageReference Include="Microsoft.NETCore.Targets" Version="3.0.0" PrivateAssets="all" />

尚自分の場合、Version: 5.0.0を追加した。

WIP:VisualStudio2022で利用している拡張機能

VisualStudio2022で利用している拡張機能をまとめる

制作者の方々が順次対応されているので、徐々に更新していく予定。（いつもありがとうございます🙇）

OpenonGitHub

marketplace.visualstudio.com

VSColorOutput64（64bitになったためか後ろに64がついた）

marketplace.visualstudio.com

GitDiffMargin

marketplace.visualstudio.com

ProductivityPowerTools2022

marketplace.visualstudio.com

Open in Visual Studio Code

marketplace.visualstudio.com

TypeScript 4.7.4 for Visual Studio

marketplace.visualstudio.com

Github Flowをベースに、メンバーのパワーにグラデーションのあるチームに優しい仕組みにするケースを考える

branch model

Github Flow の機能開発branch部分は好きに運用しろというスタンスだと思っている

機能開発branchは、あくまで機能開発branchであって、これらはdeploy可能であるかどうかはわからない。 一方でmainはavailableであることを保証する必要がある。

という前提に立って考えると、PRでいきなりmain branchにはmergeさせたくなくなる。

そもそもgithub flow is 何

  • 機能開発branchとmain branchの体制で運用する。
  • PRでのレビュー必須（Approvedは可能であれば複数が望ましい）
  • テストコード必須 or 推奨
  • 機能開発branchがMerge Ready になる とは [ devでの検証が取れている ] ではなく、[ 機能開発branchの内容をprodにdeployしてから、検証OKのフィードバックを受け取るところまで完了している ]である。

が、主だった特徴に思える。

release branchが欲しくなる

mainと機能開発branchの構成の場合、機能開発branchの検証タイミングとrebaseタイミングが煩雑になりがちになる。
そして、チームメンバーのスキルにグラデーションがある場合

手の速いプレイヤーがガンガンリリースを切っていくことでmainのコミットとtagはどんどん積みあがる。
すると、手の遅いプレイヤーは再rebase作業が重なり、しんどい状況になる。

という課題が発生した。
そのため、

  • mainのheadからrelease branchを必ず作成し、以降のReleaseのための準備やFixについてはrelease branchにコミットを積む。
  • 存在出来るrelease branchは最大1つだけ。（= releaseの追い抜きが不許可になるため、再rebaseを行うストレスが軽減される。）

とした方がprodにdeployする資産を作る際のrebase漏れや検証途中での再rebaseの負担増が起きにくく、(少なくとも我々にとっては）筋が良いようだ。
逆に開発チームがハイパワーな場合は、rebase業を忘れることはなく、複数回のrebase業が苦にならなかったり、そもそも開発スピードが速いため、rebase業をそれほど行わずにガンガンreleaseを切っていけるんだと思う。

PRレビューを小口化するためのルールについても形式化したくなる

機能開発branchからSub branchを切るかどうかの判断は、PRを分割するかどうか(=PRのサイズが大きくなるかどうか)という観点で行われることが望ましい。
形式化すると無駄なbranch操作が必要になるので、生産性には悪影響がある。
一方で、チームのパワーにグラデーションのあるケースだと、所作を決めたほうが混乱が少ないようだ。

所作をある程度固定化しつつ、生産性のために一部の所作は省略しても良いよう、branch modelを書いてみることにした。

customed branch model figure

customed-git-flow

about customed branch model

前提条件

  • 機能開発branch、release branch、main branchの体制で運用する。（左記のbranchは必ずbranchのフロー上登場する。）
  • PRでのレビュー必須（Approvedは2以上必要）
  • テストコード必須 or 推奨
  • release branch を作成する = Staging へのdeploy＆検証 Ready = 機能開発の実装自体はこのタイミングで閉める。
  • release branch は１度に最大１つだけ作成出来る。但し、複数機能開発branchをMergeすることは可能。
  • release branchに機能開発branchをMergeするタイミングで、必要に応じてrebaseが必要。
  • prodへのdeployが完了し、prodでの検証OKのフィードバックを以て、main branchへのmerge、releaseのpublish、Tagの追加がReadyとなる。

feature integration branch は 形式化したい人向けのoptionalなルート

  • 機能開発を統合するようなbranchを作成し、PRを小口で作成し都度mergeしていく(prerelease0.1 => 0.2 のフロー)
  • 機能開発branchで開発していて、先にレビューしてもらいたいケースが出てきた場合に、sub branchを作成し、PR作成⇒レビューを行い、merge readyになったタイミングで機能開発branchにmergeを行う（prerelease0.3 => release 1のフロー）

両者はやっていることは同じなので、本質的には後者のみで構わないはずだが、形式化を好む人は前者での作業のほうが心が安らぐようなので記述した。
一方で、合理性を好むタイプの人は後者での作業を従来通り行ってくれれば良い。

尚、機能開発branchのサイズが小さければsub branchを切らなくて良い(prerelease0.2 => 0.3 のフロー)

開発中の機能をmainに合流させたいケースを考える

前提としてエッジケースなので、図には記載しない。
開発が割合長期に渡る等の理由で、prodの稼働等が未達だが、mainにコードを合流させたいというケースがリアルワールドでは起こりうる。
実際問題、開発を統合するためのbranchを育てる方法も度が過ぎれば、PRのレビューやテストのカロリーが許容範囲を超えてくることはままある。

開発中の機能からmainへの直接PR作成＆mergeについては

  • 現在稼働中の資産への影響がないこと
  • 現在release branchが生存中ではないこと

を満たしている場合のみ許容するのが筋が良いように思える。

release branchの存在意義について

ケースによってはrelease branchが冗長になりがちではあるものの、機能開発branchのrebaseを忘れた状態で検証を行ってしまって時間をロスするといったケースを防ぐことが出来るというメリットが存在する。 一方で手数は増えるため、開発チームのパワーと相談して決めるのが筋が良いように感じた。（少なくとも、2021年現在の自分のチームでは、必要なように思う。）

また、release branchは複数の機能開発branchをMergeする役割も担うことになる。（図で表現されていないけれど……。）

まとめ

こんなもの考えなくても完全にWayに則って進行できるのが一番ローコストで良い。

と思う一方で、チームのパワーに合わせてまずはミスが出にくい仕組みにして一歩ずつ進めていくしかないという気持ち。

Swashbuckle.AspNetCore.SwaggerでSwaggerUiのRootPathを変更する

ASP.NET Core MVC Swashbuckle

Swashbuckle.AspNetCore.SwaggerでSwaggerUiのRootPathを変更したい

前回のユースケースで、PathBaseを利用してWebApiのRootPathを変更した場合、SwaggerUi自体のRootPathも変更が必要になります。

UseSwaggerUIのSwaggerUiOptionsのRoutePrefixにサブディレクトリを設定する

個人的にSwashbuckleに関連するConfigureをまとめてるのは以下です。 github.com

UseSwaggerUIのSwaggerUiOptionsのRoutePrefixにサブディレクトリを設定するコード例

github.com

                    .UseSwaggerUI(option => {
                        option.SwaggerEndpoint(swaggerUiSettings.Url, swaggerUiSettings.Name);
                        if (swaggerUiSettings.RoutePrefixSettings.IsOverridable) {
                            option.RoutePrefix = swaggerUiSettings.RoutePrefixSettings.RoutePrefix;
                        }
                    });

settingによって切り替えるコードにしているのでswaggerUiSettings.RoutePrefixSettings.IsOverridableの検査が入っていますが、option.RoutePrefixに、サブディレクトリを設定することで、SwaggerUIのRootPathを変更することが可能です。

次に

  • SwaggerUiからrequestを投げるエンドポイントのRootPath（Swagger a.k.a. OpenApi的にはServersに関するSettings）

の設定を行っていきます。（続きます。）