自分がこれまで携わって来たプロジェクトでは、ソースコードのコミットというものをあまり重視していません。代わりにgithubなどのプラットフォーム側が提供しているプルリクエストを重視しています。各種管理もプルリクエスト単位で行なっています。
プロジェクトによって違いはありますが、コミットやプルリクエストは以下のようなルールを基準にプロジェクトに応じた調整を行なって運用することが多いです。
前提
プルリクエストはコンパクトに
プルリクエスト単位での管理を行う前提として、1つのプルリクエストに含まれる内容をなるべく少なくするようにしています。
プルリクエストをコンパクトにすることにより、コミット単位ではなくプルリクエスト単位で管理できるようにしています。
このあたりは長くなってしまったので別記事にしました。
masterブランチへの直接pushは禁止
プルリクエストを出さずにmasterブランチへ直接pushすることは禁止しています。
例え開発リーダーのような立場の人でも、誰かのレビューを通らなければ絶対にmasterへマージすることはできません。
コミットルール
コミットに関してはかなり自由なルールにすることが多いです。ただし、
帰宅前にはリモートプッシュ
コミットの順番をrebaseなどで操作しない
の2点はルールに盛り込むことが多いです。
コミットのタイミングは開発者の自由
コミットするタイミングには下の一点を除いて制限を設けていません。離席する時にコミット、会議の前にコミット、ブランチを変える時にstashの代わりにコミット、などなど担当者におまかせしています。
日々仕事を終える時にコミットしてプッシュする
その日の仕事を終える時には必ずコミットを行い、さらにリモートブランチにプッシュしてもらっています。
これは、担当者が仕事を休む場合などに作業を引き継げるようにしておくためです。
rebaseなどでコミットの順番を操作することを禁止
rebaseなどでコミットの順番を操作することは禁止しています。ブランチで開発中にmasterに修正が入った場合もrebaseではなくmergeで対応しています。
一番の目的は後々作業の流れを把握できるようにするためです。
どのような順番で開発していったのか、いつmergeを行なったかなどをそのままコミット履歴として残しておきます。これを、不具合が発生した場合の調査のネタとして利用したり、品質分析などを行う時の分析材料として利用したりしています。
経験上mergeのタイミングで不具合や品質低下が発生していることが多いので、mergeが行われたタイミングは特に重視しており、開発フローの見直しなどに活用しています。
また、gitに慣れていない人がプロジェクトに参画することは多く、誤った操作でブランチを壊してしまうといった事故が結構多いのですが、rebase禁止にしておくとその事故が多少減る効果もあるようです。
コミットログの記入ルールは最小限に
コミットログについての記入ルールはほとんど設けていません。ツール連携のためにプルリクエストに紐づくタスクのID(チケット番号)を入力するという程度のルールを設けることは多いです。
プルリクエストルール
プルリクエストの際にはいろいろなルールを設けています。
十分な品質のものをmasterブランチにマージする
素早いマージを心がける
と行った点をルールでは重視しています。
テストの実施後にプルリクエスト
担当者はプルリクエストを出す前に必ず開発箇所のテストを実施します。開発したものが設計通りであることを担保した上でプルリクエストを行います。
テストの方法はプロジェクトによって様々です。JUnitなどにより完全に自動化する場合もあれば、自動と手動のハイブリッドで行うこともあります。
また、品質が上がらないプロジェクトでは、テスト実施後にエビデンスの提出を求める場合もあります。
レビュワーは、テストが行われ設計通りであることを前提としてレビューを行います。大半のプロジェクトではレビュワーがボトルネックになりがちなので、素早くレビューをパスさせるためです。
開発初期であったり、難易度の高いプルリクエストであったりする場合には２重にチェックを入れることもあります。
自己チェックリストの確認
担当者はプルリクエストを出す前に自己チェックリストの確認を行い、チェックリストの内容をパスしていることを担保した上でプルリクエストを出すようにしています。
チェックリストには例えば、
- lintなどから警告が出ていないこと
- DBの場合、整合性や大量のクエリを出していない(n+1問題など)ことなど
- 個人情報の扱い（暗号化など）が適切であること
- プルリクエストを出す前に差分を確認すること
- プロジェクト固有のチェックポイント
という感じに、単純なものから性能面、セキュリティ面などプロジェクトが重要視しているものをチェック項目として載せています。
また、レビュワーから多く指摘が上がるポイントについては都度チェックリストに追加していきます。逆にチェックリストの内容が膨らみ過ぎてチェックの時間がかかり過ぎないように、不要なものをチェックリストから除外することもあります。
レビュワーはやはり、チェックリストの内容には問題がないことを前提としてレビューを行います。
残課題はあきらかに
本来は仕様が確定してから開発を行いたいものですが、現実には仕様が未決定や未確定のまま開発をせざるを得ないこともあります。
このような状況でプルリクエストを行う場合は、TODOコメントなどで今後の課題として残すとともに、redmineなどで残タスクとしてチケットを切ることにしています。
レビュワーは原則プルリクエストを通す
レビュワーはプルリクエストを通すために行動するように心がけています。
機能が不足している、性能要件を満たしていない、など問題が明確である場合には指摘を行います。
しかし、格好良い悪いといった主観的要素が強い理由では指摘を行わないようにしています。
工数を抑えるという目的もありますが、開発者のモチベーションをむやみに低下させないようにするという意図が強いです。
プルリクエストの場で指摘しないような事柄については、別途勉強会を開くなどして議論するようにしています。
レビュワーはプルリクエストを優先する
レビュワーはプルリクエストのレビューを最優先で片付けるようにしています。
レビュワーが専任であることは少なく、その他のタスクと掛け持ちすることが多いと思います。そうなるとついつい自分自身のタスクを優先してしまいがちですが、レビュー依頼が来ている場合には極力レビューを優先するようにしています。
プルリクエストがいつまでもマージされないと開発やテストが足を引っ張られてしまい影響が大きいため、とにかくレビューは急ぐようにしています。
また、レビュワーがレビューを優先しやすいように、レビューに掛かった工数を考慮してタスクの調整を行うようにもしています。レビュワーがレビューの分余分に残業を強いられるといったことがないように気をつけています。
プルリクエストを出す時のコメント
プルリクエストを出す時のコメントは、あらかじめテンプレートを用意しておきそれを埋めていく形にしてあります。大体以下の事を記入するようにしています。
- プルリクエストに紐づくタスクのID(redmineのチケット番号など)
- プルリクエストの内容
- テストやチェックリストの実施状況
- レビュワーへ伝えたい注意事項や重点的に見てほしい箇所など
- 残課題
レビューのやりとりは会話のように行う
時折、レビューの指摘コメントに対してただ「直しました」とだけコメントされることがあるのですが、これはNGにしています。
例えば、
「この書き方だとselect文が大量に発行されて性能上の問題があるようにみえます。」
とコメントすると返信で、
「直しました。」
とコメント返しを受けることが結構あります。
このコメントだと、実際に問題があったから直したのか、単に指摘されたから良くわからないけど直したのか、この修正が妥当なのかなど、修正者の意図が掴めず困ってしまいます。
そのため、
「確認したところレコード数分select文が発行されていたので、1クエリになるようにXXを○○に変更しました。ログで1クエリになっていることは確認済み、ユニットテストもパスしています」
のような感じにコメントすることをルールにしています。
コメント