ansible-lint にPull Request送ってみた

Ansible

はじめに

こんにちは、最近パンを焼いている者です。

これは「Ansible Advent Calendar 2022」 7日目の記事です。Advent Calendarがあるとブログ書く気になるので、ありがたいイベントです。

Kubernetes Advent Calendarにも参加したので、よければご覧ください〜。

ansible-lint と 私

ansible-lintは、その名の通りAnsibleのlinterです。PlaybookやRoleがコード規約に則って書けているかチェックできます。コード規約は、rules として独自に定義することもできますし、一般的なものはデフォルトルールとしてあらかじめ定義されています。

某社が提供するRed Hat Ansible Automation Platform (AAP)でも、AAP 2.2からansible-lintがTech Previewとして含まれるようになりました。

2022年は私にとってコードを書くタイプのOSSコントリビュートを頑張った1年でした。そのうちansible-lintのバグを修正したことについて共有させてください。なお、記事の中で紹介するバグ修正がansible-lint v6.1.0で取り込まれたため、載せるソースコードはその直前の ansible-lint v6.0.2 時点のものです。ご注意ください。

私がansible-lintで初めて修正したバグで、かつ一番大きかったのがこちらです。デフォルトルールの var-naming のバグです。変数名がコード規約に従っているかチェックするルールですが、block 内で宣言した変数に対してうまく動いませんでした。根拠なく、なんかいけそうな気がしたので、こちらに取り組むことにしました。

Variable naming standards different for tasks and blocks · Issue #1895 · ansible/ansible-lint
Summary Using uppercase variables in tasks are reported as `var-naming: Task defines variables within 'vars' section that violates variable naming stand...(続く)

(余談) good first issue の活用

コントリビュートしてみたいけど何から始めればいいかわからない場合、good first issue を活用するのもよさそうです。Issueのラベルではなくそういう名前のWebサイトです。

Good First Issue: Issues for your first open-source contribution
Making your first open-source contribution is easier than you think. Good First Issue is a curated list of issues from popular open-source projects that you can...(続く)

人気のあるプロジェクトや新たなコントリビューターを歓迎しているプロジェクトのIssueから、good first issueラベルがついた比較的取り組みやすいIssueをまとめてくれています。言語ごとにフィルターもできて便利です。

調査の一部始終

ansiblelint._internal.rules.BaseRule.getmatches()

anbsible-lintでルール違反を検出するのが、BaseRule クラスのgetmatches()メソッドです。

for method in [self.matchlines, self.matchtasks, self.matchyaml] のところで、すべての評価用のメソッドを順に読んでいき、最後にまとめて結果を返しています。

ansiblelint.rules.match_types

以下のようなメソッドが評価に使われます。match, matchtask, matchplay はそれぞれmatchlinesmatchtasksmatchyamlから呼び出される構造のようです。今回取り組んだIssueは、tasks (およびpre_tasks/post_tasks)の中のblock内の変数名をうまく評価できていないという問題なので、matchtasks/matchtaskを見るのが良さそうです。

AnsibleLintRule.matchtasks()

個々のルールは、このAnsibleLintRule classをオーバーライドしており、matchtaskだけを独自に実装しています。そのため、呼び出し元のmatchtasksは共通と思って良さそうです。

ansiblelint.rules.VariableNamingRule

var-namingルールのバグということなので、src/ansiblelint/rules/var_naming.pymatchtask() を見てみます。

task というデータをインプットとして使い、our_vars = task.get("vars", {}) のところで変数を抜き出してきています。

いろいろデバッグしていると、そもそもmatchtaskのインプットとなるデータから、blockの情報が欠落しているというのがわかりました。従って、インプットのデータを作るところ再度確認するため、呼び出し元のAnsibleLintRule.matchtasks()メソッドに話を戻します。

再び、AnsibleLintRule.matchtasks()

さて、もう一度matchtasksを見てみると、result = self.matchtask(task, file=file) のところで、matchtaskを呼んでいます。このとき引数となっているtask の作り方が怪しそうです。

matchtaskはfor文の中で呼ばれていますが、そのイテレータは tasks_iterator = ansiblelint.yaml_utils.iter_tasks_in_file(file, self.id) で生成されているのがわかります。怪しい。

ansiblelint.yaml_utils.iter_tasks_in_file()

iter_tasks_in_file() は、yaml_utilの中にある関数ですが、YAMLファイル内のtaskをイテレートします。具体的にはraw_task, normalized_task, skipped, error のタプルを生成するようです。

サンプルのPlaybookを作って地道にデバッグしてみると、raw_tasks = get_action_tasks(data, lintable) でdataをtaskの形に変換する際に、以下のようにblockの情報が捨てられているのがわかりました。

ansiblelint.utils.get_action_tasks()

ちゃんとコメントで # Remove block/rescue/always elements from tasks list と書いてましたw。経緯はわからないですが、直前の# Add sub-elements of block/rescue/always to tasks listblock/rescue/always内のネストされたタスクを取り出してフラットにしているので、不要なblock/rescue/alwaysの要素を消しているんでしょうか。このときにblock/rescue/alwaysレベルで定義した変数は消えてしまっているので、var-namingルールでうまく扱えなかったようです。

プルリク送ってみた

前章のような調査以外にもいろいろ試しつつ、なんとか修正してPull Requestを送りました。最終的な変更内容はこちらです。別のルールのバグもついでに修正したり、テストコード追加したり、いろいろできました。普段コード書くことそんなに多くないのでとても楽しかったです(小並感)。最初は見当違いな実装をしていた上テストを書いておらず、今考えれば散々な内容でしたが、レビュアーの方が親切で助かりました。

Make sure all tasks get evaluated by matchtask including block/always/rescue and nested tasks by nishipy · Pull Request #2031 · ansible/ansible-lint
Fix #1895.

(余談) テスト駆動開発

今回の修正を行なったおかげで、他のルールの実装をリファクタリングすることもできました。リファクタリング前に不足していたテストを書いたり、リファクタリング後に漏れていたテストケースを追加したりしました。
リファクタリングのPull Requestもなんとかマージしてもらい、テストコードって大切だなあと感じたので、後日こちらの書籍を読みました。そして、これ理解してなくてごめんなさいという気持ちになりました。

おわりに

何度も心が折れそうになりましたが、無事マージしてもらえてよかったです。普段使っているツールに自分で書いたコードが含まれているってなんかロマンありますよね。来年も何かしらコミュニティに貢献できるよう頑張ります。

References

コメント