はじめに
初めてOSS(Ansible)にプルリクを出してマージしてもらうことができたので、流れをメモしておきます。
自分は普段SIerで働いていて、紙とハンコでドキュメント管理しているような職場なので、世間一般の開発の感覚を知りたいと思ってOSSにコントリビュートしてみることにしました。
これから初めてやる人の参考になればと思います。
今回出したPR
pamd: Fix AttributeError when removing the first or last rule (#60497)(#66504)
基本的なOSSコントリビュートの流れ
基本的には、上の図のような流れでコントリビュートをします。
まず、コントリビュートしたいリポジトリを自分のアカウントにforkして
そこからローカルにcloneして編集し、終わったら自分のリポジトリにpushしてからPRを出します。
共有リポジトリは権限の設定で、一般人が勝手にpushできないようになっていることが多いです。
順を追って説明
実際にやったことを説明していきます。
コントリビュートするプロジェクトを選ぶ
まず多くの人が迷いそうなのが、どこのソフトを選ぶかですね。
自分がコントリビュート対象を選ぶ時に意識したのが、下の3つです。
- よく使っているソフト
- モジュールやプラグインがあるソフト
- 大型のプロジェクト
今回は構成管理ツールのAnsibleを選びました。
よく使っているソフト
これは言うまでもないですが。
自分がよく知っているソフトの方が理解が早いし、モチベも上がります。
モジュールやプラグインがあるソフト
大きなOSSプロジェクトはソースの量が膨大で読むのにも時間がかかりそうですが、プラグイン系のコードは比較的読むソースの量が少ないので、取っ掛かりやすいです。
大型のプロジェクト
大型のOSSはルールが整っているので、初心者が流れを理解するのに良さそうです。
Ansibleの場合はIssueのテンプレートが用意されていたり、Issueを登録したらボットが勝手にタグ付けしてくたりもして、至れり尽くせりでした。
どういう流れでPRを出して、どう承認されるのかもしっかりと定義されてます。
Issueを選んで、修正する
自分でIssueを上げてから取り組んでもいいですが、ちょっとハードルが上がるので今回は他人が上げたIssueを修正することにました。
機能追加系のIssueはドキュメントとかテストコードの作成などやることが多いので、軽微なバグ修正の方が取っ掛かりやすいと思います。
今回はこれを選びました。
pamd module: cannot remove line when it is the first line of the file #60497
- pamdのモジュールを使用して、pamの一番最初の行を削除しようとするとAttributeErrorが発生する。
- 1行目にコメントでもなんでも入っていれば、エラーにはならない。
わりと簡単に直せそうにもかかわらず、半年間放置されていて直される気配がありませんでした。
まずは自分のリモートリポジトリにforkしてから、ローカルにcloneして修正に取り掛かります。
バグの原因は単純なコーディングミスだったので、コンストラクタにプロパティの初期化処理を追加しただけで修正完了しました。
直すべきファイルが他にないか？
ソースコードを修正して動くようになったはいいけど、本体のソース以外にも修正すべきことがあったりします。
テストコードの修正
Ansibleはユニットテストにpytestを使用しています。
今回はバグの修正内容を検証するテストコードを追記しました。
恥ずかしながらテストコードを書くのは初めてだったんですが、先人が書いたテストコードを読みながら2種類のテストコードを追加できました。
- PAMの最初の行を削除するテスト
- PAMの最後の行を削除するテスト
それまでのテストコードでは、中間にある1行を削除するテストしかしていなかったので、バグがすり抜けたようです。
書き足してからローカルでテストを実行すると、修正前のプログラムでは両方NGになるが、修正後のプログラムでは両方OKになることを確認できました。
チェンジログ
他の人のを見るとYAMLのchangelogを付けてる人が多かったので、見よう見まねでつけてみました。
これがどう使われるのかは、よくわかっていません。
bugfixes:
- "pamd - Bugfix for attribute error when removing the first or last line"
ソースコード中のドキュメンテーション
pydocなんかを使ってソースコード中にドキュメントが書かれていたりするので、パラメータを追加したり仕様を変更したりした場合は変更が必須になります。Ansibleの場合は自動のsanity testでチェックしてくれるので、万一漏れていても気付けるようになっています。
ソースコードのコメントが自動生成でそのままドキュメントになったりすることもあるので、ちゃんと書きましょう。
今回の場合は特に変更すべきところはありませんでした。
ここまで終わったら、自分のレポジトリにpushしよう。
プルリクを出す。
いよいよ、PRを出します。
PRに書くべきこと
PRにはこんなこと書くといいみたいです。
- PRの概要
- 修正の種類(バグ修正、機能追加、ドキュメントのタイポなど)
- 修正したファイル
- その他詳細情報など
Ansibleの場合はテンプレートが準備されていたので、それを埋めていくだけで完成しました。
自動テストが走る
Ansibleの場合はPRを出したタイミングと追加のpushをしたタイミングでShippableと連動してテストが走ります。
今回テストコードを追加していたのでちゃんと通るかドキドキでしたが、無事30分後ぐらいにPassedになっていました。
コメントとレビューをもらう
さっそく他のコントリビューターからコメントがつきました。
shipit
Thanks for the fixes!
訳：問題ない。修正ありがとう！
shipitというのはレビューOKを意味するGithubのスラングです。
Ansibleのレポジトリを巡回しているボットのAnsibotはこのshipitをカウントして、自動マージするか判断しているらしいです。
同時にapproveもしてくれていました。
追加で変更があった場合
実は今回、一度テストコードなしでPRを出した後にそれを指摘されて、直しています。
一度PRを出した後に修正したい時は、単純にローカルから自分のリモートリポジトリに再度pushすれば自動的にPRの画面に反映されます。
マージされるまで待つ
あとは待つだけです。
Ansibleのプロジェクトでは、PRがマージされる条件がいくつかあるようです。
- Memberの人がレビューしてマージする
- 一定以上の
shipitが付いていた場合、Ansibotが自動でマージしてくれる
Memberというのは、OSSをメンテしているRedhatの開発者(?)の人たちです。
1週間ぐらい待ったら、無事Memberの人がマージしてくれました。
Backport PR
マージされて喜んでいたら、Memberの人からこんなコメントが付きました。
Please create a backport PR for this to be included in previous versions.
訳：以前のバージョンに反映されるように、バックポートPRを出していただけますか。
バックポートは、新しいバージョンでの変更を古いバージョンに取り込むことらしいです。（初めて知りました）
バックポートとは、ソフトウェアの新しいバージョンの機能や、そのバージョン向けに開発されたプログラムなどを、古いバージョンで利用できるように移植すること。
バックポートとは - IT用語辞典 e-Words
Ansibleでは、めちゃくちゃ丁寧にBackport PRの出し方が説明されていたので、書いてある通りにコマンドを入れればすぐ完成しました。
具体的には、マージ後のメインブランチから今回の変更をgit cherry-pickして、一つ前のstableバージョンにマージするPRを出す作業です。
今回出したBackport PR
[stable-2.9] pamd: Fix AttributeError when removing the first or last rule (#66398)
これで、今回の修正作業はすべて終わりました。
執筆時点ではBackportはマージされていませんでしたが、そのうちマージされるでしょう。
感想
- 不特定多数の人が自由に参加しても品質を保てるように仕組みが整備されているところは感動しました。
- 暗黙のルールとかあるんじゃないかと不安でしたが、他のPRとかIssueを見ながらやれば大丈夫でした。
- 他のコントリビューターやメンバーがとても優しくて助かりました。