Githubでの開発 - Issue, Commit, Pull Request, Mention, Code Reviewに関する基本的なルール

ゴール

「 チーム で 長期にわたって 生産性を上げる」

前提

みんながサービス・プロダクトについて自主的に考える組織

エンジニア全員がそれぞれオーナーシップを持ってよりプロダクトを良くすることを考える
いわゆるPM職の不在 = コードは書かずに、マネージだけする人がいない

これは組織による。(e.g. 外注やディレクター職の存在)
けれど、Wantedlyは、多少変化しつつも、より良いサービスを生み出すために、役割の程度の差はあれ全員がプロダクトについて考え責任を持ったほうが良いと考えている。

理想型

図:「青と黄色」のチーム構成が従来の縦割り+統括チーム、「緑(金)色」のところが目指すべきマイクロサービスチーム

マイクロサービスチームは、それぞれ 1 機能＝マイクロサービスを担当し、それに関わること全てを担当する（カスタマーサポート・内部ツール開発・DB スキーマの管理・テストなど）
オペレーションは、ミーティングやチケットでの依頼して行うものではなく API コールによって行われる

これが良い理由

専門性によって組織を分けるとチーム同士でミーティングや調整を何度もする必要が出てくる。これは生産的ではない。
専門性によってチームを分けると、次第に仕事の「義務」の要素が強くなり「情熱」を持って取り組みづらくなるという例もある。

State of the Art in Microservices by Adrian Cockcroft より

そのために

人に許可をとらなくても自主的に試してみることができる環境
自分だけじゃなく、チームとしての生産性を考えられる組織

を目指してます!

具体的にできること

読みやすいコードと、古すぎない開発環境を保っていくこと
Issue や Pull Requestに必要な情報をしっかり書く
レビュアーに少ないコストでレビューさせてあげる
お互いの作業を出来るだけ止めないようにする

などなど

これから、そのテクニックやベストプラクティスを(普通にやってても身につかないところを重点的に)紹介します。

非同期コミュニケーションのやり方

なぜ非同期コミュニケーションが重要か

TED - ジェイソン・フリード: なぜ職場で仕事ができないのか
ジェーソン・フリード: basecamp(元37signals)の創業者, 著書:「小さなチーム、大きな仕事」

Q:「仕事に集中したいときにどこに行きますか？」

オフィスと答える人はほとんどいない - 働くための最適な環境をつくりあげようとしているのにもかかわらず
答えとなるのは、家の個室、トイレ、電車、飛行機、カフェ、図書館など

原因: どんないい状態であろうとも、作業が強制的に中断してくるものの存在

同僚の呼びかけ、上司のチェック、ミーティング

Creativeな仕事は睡眠に似ている

何度も起こされながら、合計8時間睡眠をとっても、しっかりした休息がとれない
何度も作業を中断されながら、合計8時間仕事をしても、しっかりしたプロダクトは作れない

現実的には

営業/広報などミーティングのみによって何かが進む作業役職も存在する

例: 営業は資料の質が良くても、ミーティングで契約がとれなければNG、逆に質が悪くても契約出来たらOK

すぐに止めないといけないような自体、止めないとほとんどの物事が進まなくなる自体も存在する

例: クリティカルなバグ、初期の設計など

なので、別職種に理解をしてもらうことや、状況に応じてうまくバランスを取ることとが大事

現実的な解決策

必要のない部分は、肩たたきやSlack MentionなどをしてすぐにResponseを求めるのをやめよう!
mentionしなくても答えが返ってくるという前提環境をみんなでつくろう！

2の前提環境がないと1の方法で仕事が進まなくなるので、2を実現するのが大事！

やるべきこと: Github Notification の活用

を定期的に見るだけ!!

朝
昼ごはんから帰ったあと
夜帰る前など

注目：unreadではなくparticipating
(自分は関係ないのはreadすらしないようにしている)

達成されること

Participating は Github上で、

自分が作った
assign された
@mention された

Issue/Pull Request のみ見れる

何が良いか

ちょっと相談したい
知っていてほしい
いい案があったら聞きたい

みたいなときに気軽にIssue/Pull Requestで mention しても大丈夫

ベストプラクティス

見逃している人がいた場合、Pushしたい場合の例外ルールを作る

丸1日以上レビューアーが放置している場合はメンションOK
合意している時間（朝9:00-10:00）はメンションOK
それをしないとほかが始まらないような根本に関わるような相談はしてOK

もっとテクニックが知りたい人は

GitHubで自分が関係しているIssueを見逃さないようにするためのページ一覧

参照

Pull Requestを出すまでのフロー

前提: gitの基本的な使い方は理解し、git configのname, emailなどの基本的な設定はすでに終わっているものとする。
cf. Wantedlyで開発するときにとりあえず設定してもらう事一覧

標準的なフロー

Issue を作る (コードを書く前に十分に議論、設計をする)
Branch を作る
コードを書いて幾つか Commit を重ねる
Draft Pull Requestを出す。 (この時何が終わってないのかも書くと良い)
レビュアーをAssignする。 (相談事があるときはここで相談する)
コードを書き進める
終わったら、Draftを外し、レビューアーをmentionする
レビューを受け、コードを修正する
レビューも終わり、テストが通っていたらマージされる
デプロイ
デプロイ後の確認

※ 問題の大きさが小さい時は適宜省略して運用してます

1. Issueを作る

大きなプロジェクトになる場合、まずはIssueを立て、しっかり方針や、やる内容、設計などを検討すること。

大きな議論はPull Requestよりも最初のIssueの時点でやってしまうほうが良い。コードを書かれると根本の指摘がしにくくなる。また、コードの指摘とそもそもの議論が混じるととわかりにくくなる。

Bugも、Errorも基本的にはIssueになっているので、すごく細かいPull Request以外はすべて元になるIssueがある状態が保たれるようにすると良い。

良い Issue の書き方

専門知識がなくても、わかりやすいタイトルを付ける
WHYを書く
今ここにいない人のことも考える
新入社員、他のチームで近い部分を変更する人、1年後のすでに他人となった自分
git blameや検索で、後で見られることも実際何度もあった

例

Title: Terraformの導入  <- x

Title: AWSの設定をコードで管理できるようにする <- o

Terraformのことは内容に書いていく。具体的には以下。

## WHY
- AWSでコンソールポチポチを無くしたい
- 属人性を排除したい

## WHAT

[Terraform](https://www.terraform.io/)を使ってAWSの設定をコード化する
まずは、RDSの設定から置き換える。

2. Branchを作る

ブランチを切る

git checkout -b awakia/facebook_ads

<ユーザー名>/<ブランチ名>でブランチを切る。
ブランチ名は、commitメッセージとは異なり、add_hogehoge, fix_hogehogeみたいな文法的に正しい名前より、キーワードを_でつなぐみたいな名前のほうが、補完も効きやすくわかりやすい。add_やfix_などで4文字取るのは無駄だしprefixがかぶりやすくなるので補完の妨げになる。

3. コードを書いて幾つか Commit を重ねる

(注) ここでの話は、スピード感を落とさず現実的に運用していくために、現在、Wantedlyで良いと思われている方法です。オープンソースや他社などお作法が違うところはあるので、そこではそちらに従いましょう。

コミットの単位

簡単にいえば、 大きすぎるよりは小さすぎるほうが良い

オープンソースだと、1 機能 = 1 Commit = 1 Pull Requestの様なことが求められたりする。しかし、会社でやる場合は、そこまで大きい必要はなく、どんどん小さくコミットして、1つの機能のまとまり = 1 Pull Requestぐらいが良い。

理想的には、以下の様なことが守られていると良い。

1つの"作業"のまとまりである
逆に言えば、複数の作業を同時にしない
特に、リファクタと機能追加を同時にしない
どこをとっても、実行可能である
実際、Typoとかちょっとしたバグが有るときは、気にせずに、FixのCommitを重ねて良い
機械的に生成した時は、生成のコマンドだけで1つのコミットにして、書き換えた部分は別コミットにする

コミットメッセージの書き方

例: Add deleted_at column to users, Fix #1234

1文字目は大文字、動詞の原形を用いる
ピリオドは付けない

詳細が書きたい場合は、空行を入れたあとに何行でも書くことができるが、詳細を書かないといけないぐらいならもっと小さくコミットしておいたら良いことが多い。
また、実際コミットメッセージの詳細はあまり読まれないので、重要な詳細はPull Requestに書いたほうが価値が高い。コミットに対応するPull Requestは、git openprコマンドの設定をすれば簡単に見つけられる。

参考

How to Write a Git Commit Message - Chris Beams
http://chris.beams.io/posts/git-commit/
Writing good commit messages · erlang/otp Wiki · GitHub
https://github.com/erlang/otp/wiki/Writing-good-commit-messages
提言: コミットメッセージの一行目には要求仕様を書け
http://qiita.com/magicant/items/882b5142c4d5064933bc

TODOメッセージの書き方

hoge.rb

# TODO(awakia): Remove this method after 2015/08/01
def hoge
  @fuga += 1  # NOTICE(awakia): To avoid hoge gem bug, See #12345 
end

#のあとに空白を一つ開ける。
同じ行にコメントを書く場合は、2つ空白をあけるのがGoogle流。少し読みやすいし、" #"で検索すると確実にコメント行を見つけられるという微妙な嬉しさがあるので僕は好き。Wantedlyでは両方使われている。1つでも2つでも良い。

コメントの出だしは、TODO(ユーザー名): , WARNING(ユーザー名): などにすると良い。恒久的に使えるコメントは、こういう形式じゃなく普通に

# comment message

で良い。ユーザー名には基本的にはTODOを書いた人(自分)を入れる。担当者が明確に決まっている場合のみ、その人を書いておく。

基本的にTODOやNOTICEなどは、放置されたり、原因が修正されていたりする。そのため、後々になってこのコードをさわる人が判断できる情報:

誰に聞いたらこの問題がわかるのか
いつまでにやるTODOなのか
詳細が書かれたリンクやIssue ID

などを書いてあげると親切。

4. Draft Pull Requestを出す。

コードが書き終わらないタイミングでもPull Requestを出してしまうことで、早い段階で周りに自分のやっていることを示し、方向修正やアドバイスを貰うことができる。

Pull Requestの先はmainでOK。ブランチ戦略は基本的に Github Flowと呼ばれるもの。(iOSのプロジェクトで複数チームが開発する場合はGit Flowのほうが良さそう)
大きなプロジェクトはそれ用のbranchを切り(e.g. rails4)、そこにどんどんPull Requestを送る形にする。

GitHub Flow: https://gist.github.com/Gab-km/3705015

5. レビュアーをAssignする

上記のことを実現するために、このタイミングでレビューアーをassignしてしまうと良い。

6. コードを書き進める

この工程が長くかかる場合や、レビューが議論になってしまってボールがどちらにあるのかわかりにくくなる場合がある。

その場合、assigneeをauthorの方に付け直すと良い。(これは多くの場合レビューアーの方がやる)

議論が複雑になってしまった場合、こうやってassigneeを付け替えあってボールがどちらにあるのか明確にするのが、ベストプラクティス。

7. 終わったら、Draftを外し、レビューアーをmentionする

AssignのタイミングでNotificationがすでに飛んでしまっているので、またメンションすると良い。

@awakia (xxの実装も終わりました。)レビューお願いします。

一度見てもらっている場合は、新しく何をしたかも書いてあげると親切。

ワンポイント: Pull Requestのマージと同時にIssueを閉じる方法

commitメッセージ、Pull Requestのメッセージ、もしくはPull Requestのコメントで、issue idの前にfixかcloseが含まれていると、Pull RequestがマージされたタイミングでIssueも同時に閉じられることになる。

なので、完成したタイミングでIssueが閉じれる場合、どこかに、

Fix #1234

To close #3456

などと書くと、Issueの閉じ忘れが減って良い。

8. レビューを受け、コードを修正する

ロジック的に正しいか、読みやすいか、スタイルガイドに従っているかなどをチェックする。
ここでは、一段上のレビューをするために知っておくと良いキーワードを一つ紹介する。

Code Smell: コードの不吉なにおい

コードの臭い - Wikipedia
リファクタリング「コードの不吉なにおい」

・重複したコード
・長すぎるメソッド
・巨大なクラス
・多すぎる引数
・変更の発散 ( 変更箇所が特定できない )
・変更の分散 ( 変更を行うたびにあちこちのクラスが少しずつ書き換わる )
・属性、操作の横恋慕 ( あるメソッドが自分のクラスより、他のクラスに興味を持つ )
・データの群れ ( 複数個のデータがグループで出現 )
・基本データ型への執着
・スイッチ文
・パラレル継承 ( 新たなサブクラスを定義するたび、別の継承木にもサブクラスを追加しな
ければいけない )
・怠け者クラス ( 十分な仕事をしないクラス )
・疑わしき一般化 ( いつかこの機能が必要になるさ )
・一時的属性 ( インスタンス変数の値が、特定の状況でしかセットされない )
・メッセージの連鎖 ( 連鎖の構造に依存 )
・仲介人 ( メソッドの大半が委譲しているだけのクラス )
・不適切な関係
・クラスのインターフェース不一致
・未熟なクラスライブラリ
・データクラス
・接続拒否 ( 継承した操作、属性が利用されず混乱が引き起こされている )
・コメント ( 非常にわかりにくいコードを補うためのコメント )

ベストプラクティス

やり過ぎもいけない
cf. 重複したコード vs 疑わしき一般化
Railsの場合は特に、名前の付け方、過度なメタプログラミングをしない(DSLを作らない)ことに注意

=> その設計が解消する複雑性と、もたらす複雑性を比較しよう。

答えは一つじゃない！

=> 疑問に思ったら二人で悩まず、みんなに意見を聞いてみよう。

その他: 宗教論争は出来る限りしない

例えば、rubyのカッコを着けるか着けないか

=> どちらでもいいものは、どちらでもマージしている。
=> ファイルの前後見て、統一感が出るようにしよう。

逆に明確なメリット・デメリットがあるものはソース(情報源)とともに指摘してあげよう。

その他レビューのコツ

別記事コードレビューの際に気をつけることにまとめました

9. レビューも終わり、テストが通っていたらマージされる

大きさや、その後の確認の必要性により、以下の2通りのどちらかが行われる。

マージしてもらう
LGTMのみもらい、マージとデプロイは自分でやる

基本的には勝手にマージしないこと。
マージされたら出来る限り早くデプロイしてしまうと良い。

10. デプロイ

基本

$ bundle exec cap prod deploy:diff

どんな変更が出るか確認。
特にDBの変更などがない場合

$ bundle exec cap prod deploy

この後、出した部分のチェックや、HoneyBadgerでエラーが飛んでないかなどをケアする

Migrationがある場合

基本的にカラムやテーブルの追加Only、または、削除Only にする

追加(add)がある場合

$ bundle exec cap prod deploy:pause_before_launch

go/stop と聞かれたら、別Windowで

$ bundle exec cap prod run “rake db:migrate”
$ bundle exec cap prod run “rails r lib/batch/hogehoge”

と必要な作業を行って、元のほうで

go

と打つ。

削除(delete)がある場合

$ bundle exec cap prod deploy

で普通のデプロイをした後

$ bundle exec cap prod run “rake db:migrate”
$ bundle exec cap prod run “rails r lib/batch/hogehoge”

と必要な作業を行う。

Renameなどが必要な場合

影響が大きい、使用しているユーザーが多い等の場合、Renameをせず、

まず追加をし、
次に削除を行う

そうではない場合、追加と同じ作業を行う

QAにmain以外のブランチを出す場合

$ bundle exec cap qa deploy[your_name/branch_name]

つまり、普段は、

$ bundle exec cap qa deploy[main]

の命令が流れている。

11. デプロイ後の確認

実際にProductionで意図したとおり機能が動いていることを確認する
HoneyBadgerでエラーが飛んでないか確認する

参考:「完璧なプルリクの書き方を教えるぜ」

基本的には、わかりやすく、礼儀正しく、お互いをリスペクトして、喧嘩しないようにやりましょう。

指摘に関しては、下手に疑問形にすると嫌味っぽくなるし、素直にこうしたほうが良いと言ったほうがわかりやすくて良い場合も多い。
このドキュメントが前提としているリモートとは別で、同じ場所で働いているので、うまくバランスをみて使ってください。

ゴール

前提

理想型

これが良い理由

そのために

具体的にできること

非同期コミュニケーションのやり方

なぜ非同期コミュニケーションが重要か

現実的には

現実的な解決策

やるべきこと: Github Notification の活用

達成されること

何が良いか

ベストプラクティス

もっとテクニックが知りたい人は

Pull Requestを出すまでのフロー

標準的なフロー

1. Issueを作る

良い Issue の書き方

例

2. Branchを作る

最新のmainにする

ブランチを切る

3. コードを書いて幾つか Commit を重ねる

コミットの単位

コミットメッセージの書き方

参考

TODOメッセージの書き方

4. Draft Pull Requestを出す。

5. レビュアーをAssignする

6. コードを書き進める

7. 終わったら、Draftを外し、レビューアーをmentionする

ワンポイント: Pull Requestのマージと同時にIssueを閉じる方法

8. レビューを受け、コードを修正する

Code Smell: コードの不吉なにおい

ベストプラクティス

その他: 宗教論争は出来る限りしない

その他レビューのコツ

9. レビューも終わり、テストが通っていたらマージされる

10. デプロイ

基本

Migrationがある場合

追加(add)がある場合

削除(delete)がある場合

Renameなどが必要な場合

QAにmain以外のブランチを出す場合

11. デプロイ後の確認

参考:「完璧なプルリクの書き方を教えるぜ」