SonarCloudをGitHub Actionsに組み込んで活用したい

[berryzplus]

PR の目的

現在Azure Pipelinesの定時バッチで利用中のSonarQube解析を、
GitHub Actions のイベント駆動（＝オンデマンド方式）に変更することにより、
静的解析の結果を効果的に活用できるようにします。

具体的には、静的解析SonarCloudの結果をレビュー時の判断材料に利用できるようになります。
表示サンプル： sakura-editor/SakuraSonarTest#19

カテゴリ

• ビルド関連
  • Azure Pipelines (定時バッチのスケジュールを止めます。ソース変更はしません。)
  • GitHub Actions

PR の背景

#1476 「SonarCloudをもっと便利に使えないかなぁ」を実現するPRです。

--	解析環境	定時解析	随時解析	解析結果コメント
現状	Azure Pipelines	1日1回	なし	なし
提案	GitHub Actions	週1回	PR作成時、PR更新時、PRマージ時	あり

当初、GitHub Actionsの問題(#1486 「GitHub Actionsの構成がおかしいぜ？」)を「同時に対応する」と言っていたのですが、SonarCloud対応だけでも先行して検討すべきだと判断したので分離しました。

PR のメリット

• PRが変更した部分に対する静的解析が行われるようになります。
• PRが変更した部分に対する静的解析の結果をbotがコメントしてくれるようになります。
• テストカバレッジを計測する機構が導入されます。
• 定時解析の頻度を下げることにより、不要な解析実行を抑止できます。

PR のデメリット (トレードオフとかあれば)

• このPRの変更内容は、既存の仕組みと共存できません。
  旧来の静的解析は.NET専用のSonarScannerを使っていて、互換性がありません。
  👉 マージ時にAzure Pipelinesのスケジュール起動を止めて移行させます。
• テストカバレッジの計測結果をSonarCloud解析に連携するために、
  OpenCppCoverageのプラグインDLLを自作して使っています。
  本来的には、自作モジュールはorganization管理とすべきなんですが、めんどうなので勝手に入れてます。
• SonarCloudが検出する警告が、一時的ですが爆発的に増えます（マテ
  現在の330件が3.6kまで爆増することが分かっています。
  これは、従来のSonarQube解析が HTML/CSS や Python を対象としていなかったためです。
  警告のうち3100件くらいはHTML4より前のｇｄｇｄ規約で書かれたHTMLに対するものなので、比較的簡単に対応できると考えています。（実際、ほとんど直せました。）発生する問題を潰してからPRすることも考えましたが、コミュニティとして検討したほうが良さそうな変更もあるので、あえて「対応ナシ」で提案することにしました。

仕様・動作説明

アプリ（＝サクラエディタ）の仕様・機能には影響しない変更です。

細かいことを書いても「何それ？」になること請け合いなので、見える変更を列挙します。

• PRの作成、更新、マージの際に静的解析が実行されるようにします。
• PRの作成、更新時に静的解析の結果がPRコメントに付加されるようにします。
• 静的解析の解析対象にC/C++以外のプログラムソースを含めるようにします。
• 静的解析の解析対象にgoogletestのテスト結果を含めるようにします。
• 静的解析の解析対象にOpenCppCoverageによるテストカバレッジを含めるようにします。
• 定時解析の間隔を日次から週次に変更します。
  cron記法で 毎週金曜日16:45JST に実行されるように設定しています。
  「決戦は金曜日」なのと「定時後の愉しみとできるよう配慮」が設定理由です。（≒「根拠はない」ってことです。）

PR の影響範囲

• PR作成時に静的解析が実行されるようになります。
• PR更新時に静的解析が実行されるようになります。
  「PR更新時」とは、追加pushとforce pushの両方で走るsynchronizedイベントのことです。
• PRマージ時に静的解析が実行されるようになります。
  従来は、00:00JSTの定時実行を待つ必要がありました。
• 静的解析の完了時にbotコメントが入るようになります。
• PRやマージとは関係なく実行される定時解析の間隔が1日おきから1週おきになります。

テスト内容

今回プルリクエストの解析で使っているpull_request_target(webhookイベント)はPRされた側(Baseブランチ)に存在するワークフローが実行される仕様らしいので、PRをマージせずに検証することはできません。

テストリポジトリ sakura-editor/SakuraSonarTest にて運用想定のテストを行いました。

操作	想定結果	合否
PR作成	静的解析が実行されて、チェック一覧にSonarCloud Code Analysisが表示され、結果コメントが付く。	OK
PR更新	静的解析が実行されて、チェック一覧にSonarCloud Code Analysisが表示され、結果コメントが付く。	OK
PRマージ	静的解析が実行されて、チェック一覧にSonarCloud Code Analysisが表示される。	OK

関連 issue, PR

resolve #1476 ;
SonarQube関連のissue検索 👈 SonarQube の導入経緯を確認できます。

※ SonarCloud はクラウドサービスの名前です。静的解析ツール SonarQube をクラウドで利用できるようにしたものが SonarCloud です。「静的解析そのもの」を指す文脈では SonarQube と呼称するのが正しいのかも知れません。

参考資料

https://docs.github.com/ja/free-pro-team@latest/actions/reference/events-that-trigger-workflows
https://sonarcloud.io/dashboard?id=sakura-editor_sakura
https://sonarcloud.io/dashboard?id=sakura-editor_SakuraSonarTest
https://sonarcloud.io/dashboard?id=berryzplus_sakura
https://github.com/OpenCppCoverage/OpenCppCoverage
https://github.com/berryzplus/XmlExporter

[AppVeyorBot]

✅ Build sakura 1.0.3320 completed (commit 21e0dbf80a by @berryzplus)

[berryzplus]

ビルド成功しました。（Azure Pipelines が HTML Helpビルドで一度コケたけど・・・）

リポジトリの Settings ページで Secrets を追加します。

[berryzplus]

Re-run jobsでは設定が反映されませんでした。
仕方ないので新しいコミットを積んで再実行させてみます。

[AppVeyorBot]

✅ Build sakura 1.0.3321 completed (commit b525064208 by @berryzplus)

[sanomari]

restore-keys を指定してあげれば key に完全一致するキャッシュがなくても restore 成功する仕様と理解しました。
https://docs.github.com/en/free-pro-team@latest/actions/guides/caching-dependencies-to-speed-up-workflows#matching-a-cache-key

• SonarCloud解析を GitHub Actions に組み込むことにより、PR作成から10分ほどで「解析結果」を見られるイようになります。（ただし、キャッシュが効いてないと約30分くらいかかります。）

ここを対応したら、PR本文に書かれているキャッシュミス時の問題を回避できると思います。

また、まとめてキャッシュしているパスの更新タイミングがバラバラなので、パスごとにキャッシュを分けたほうがよさそうに思いました。

[sanomari]

SonarCloudの結果が異なる理由ってこれじゃないですか？
#368 (comment)

cppcheckの実行結果をSonarScannerに食べさせることで、cppcheckが検出した警告をSonarCloudにも表示することができるようです。

[berryzplus]

GitHub Actionsって、 on: pull request だと secrets.SONAR_TOKEN 見られんじゃんね、という話。

[k-takata]

secrets が PR から見えてしまうと、悪意のある人による攻撃が可能になってしまいますから仕方ないですね。

[berryzplus]

secrets が PR から見えてしまうと、悪意のある人による攻撃が可能になってしまいますから仕方ないですね。

仕方ないですね。

GitHub Appsに関する情報を調べてみたところ、
SONAR_TOKEN は「個人に紐付くセキュリティ情報」にあたるらしく、
世間的に「そういう情報は使わない方向にすべきじゃね？」な風潮があるようです。

「個人に紐付くセキュリティ情報」をCIに組み込んでしまうと、
そのメンバーがプロジェクトを抜けるときに謎のCIエラーが発生することになるかららしいっす。

代替はAzure Pipelines 連携のアプリを使う方法になります。
たぶん、もうちょっと時間がかかります。

[berryzplus]

SONAR_TOKEN は「個人に紐付くセキュリティ情報」にあたるらしく、

う～む、そうでもない・・・ぽい。

SONAR_TOKEN生成の流れ

1. GitHub で organization を作る。
2. SonarCloud で GitHub の organization をインポートする。
3. SonarCloud で GitHub の repository をインポートする。
   SonarCloud に Project が生成され、SONAR_TOKENが生成される。

つまり、GitHub の User に紐付いているわけではなく、
GitHub の organization / reposigory に紐付いてそうです。

さて、どうしよう

• やりたいこと＝「解析結果を SonarCloud に反映するタイミングを早くする。」
• 解析するには SONAR_TOKEN が必要だが、repository の Secrets に設定しておけば GitHub Actions から参照できるようになっている。ただし、pull request から起動されたアクションからは Secretsを取得できない（≒空になる）ようになっている。
• pull request から起動されたアクションで Secrets を取得できないと、SonarCloudに解析結果を反映できないので、このPRのメリットは半減する。PRレビュー時の判断に静的解析を利用できなくなったのでかなり痛い。
• PRレビュー時の判断に静的解析を利用できれば良いだけなら、PR発行時に sakura-editor/sakura 自体の静的解析を実行する必要はない。PR元ブランチの解析結果を見られれば良いだけなので。
• そもそも静的解析の組み込みは「誰でも気軽にレビューできる」を実現するために行おうとしているもので、「静的解析結果がないと困る」というわけではない。

分析結果から、pull request では静的解析のためのビルドを完全に実行しないように変えておいたほうが良い気がしてきました。

[AppVeyorBot]

❌ Build sakura 1.0.3323 failed (commit 5fa040effa by @berryzplus)

[berryzplus]

secrets が PR から見えてしまうと、悪意のある人による攻撃が可能になってしまいますから仕方ないですね。

テストリポジトリを作って色々試してみました。
https://qiita.com/sakura-editor/SakuraSonarTest
https://sonarcloud.io/dashboard?id=sakura-editor_SakuraSonarTest

PR元と同じHEADコミットを持つブランチをPR先リポジトリに作成してやれば、SonarCloud解析が実行された結果がPRに反映されるようになっているみたいです。

ここのリポジトリは、メンバーであれば結構自由にpushできる設定になっています。
master以外のブランチは、作成するのも削除するのも自由です。

ということで、PRのレビューで「とりあえず問題なさそう」と判断したら sakura-editor/sakura にブランチを push する運用にしてやれば、当初の目的を達成できそうです。（当初想定よりもやや時間がかかりますけど。）

ただ、PRをわざわざ fetch して push するのは何気にめんどくさいです。

なんかいい方法ないかな・・・とググって見たら「これだ！」という技術情報を見つけました。
https://qiita.com/nwtgck/items/d5b61a2dc93fbf29dfc3

かなり尖ったハナシなので説明するのがめんどいのがネックです。。。
（尖った＝最先端の、という意味。対義語の「枯れた」もメジャーな業界用語。）

[berryzplus]

現状と合うようにPR本文の修正を試みましたが無理でした。

いったん閉じて、暫定で入れようとしている変更（≒最後にforce pushしたコミット群）だけを説明する感じで新規PRを起こしたいと思います。メンバーのコメント投稿をトリガーとしてプレビューブランチを作る（≒SonarCloud解析を実行させる）機構を入れるかどうかはもう少し考えてみます。

[berryzplus]

すぐに着手しないのは Appveyor が年末から障害中っぽいからです・・・。

[berryzplus]

いったん閉じて、暫定で入れようとしている変更（≒最後にforce pushしたコミット群）だけを説明する感じで新規PRを起こしたいと思います。メンバーのコメント投稿をトリガーとしてプレビューブランチを作る（≒SonarCloud解析を実行させる）機構を入れるかどうかはもう少し考えてみます。

色々ためしてみたところ、プレビューブランチを作る必要はないことがわかりました。
しかも、試行錯誤で見つけた方法のほうが当初構想に近い直感的な挙動になるようです。

分かりやすくするために、当初含めていた変更の8割くらいを削除したんですが、
試行錯誤により追加した部分を正しく理解するのは難しいと思うので、
全体的なレビュー難易度はあまり下がらないと思います。

新規にPRを立て直しても、すんなり提案内容を理解できる可能性は低いと思うので、
ぐちゃぐちゃの状態のまま、PR本文を修正して続行することにしました。

[sanomari]

問題なさそうに見えるんですけど気になることがいくつかあります。

気になったこと

• Appveyorのビルドが走っていない。
• Azure Pipelinesのビルドが走っている。
• sonarscan.ymlに除外パスが指定されていない。

わたしの解釈

• appveyorのビルドが走っていない原因は、appveyorの除外パスに該当する変更しかないので正しそうです。
• azure pipelinesのビルドが走っている原因は、azure-pipelines.ymlの除外パス指定部分にミスがあるためと考えられます。push(=マージ)のときだけGitHub Actionsワークフローのパスを除外するよう指定が入っていて、pull request時に除外指定がありません。
• appveyorとazure pipelinesの除外指定が正しいとすると、sonarscan.ymlにも除外指定を入れるのが正しいように思います。

なんとなく、appveyorとazure pipelinesの除外指定も正しくない雰囲気なので微妙ですが、appveyorやazure pipelinesの設定だけを変えたときに静的解析を実行しても意味がないのは明らかなので、除外パス指定は入れたほうが良いように思います。

[berryzplus]

sanomari mentioned this pull request 14 分前
Azure PipelinesのPRビルドの除外パスが漏れていたのを追加する #1498

ｗ

分かりました。rebase前提で進めます。

[berryzplus]

動いてますね、azure pipelines。

[sanomari]

動いてますね、azure pipelines。

ということは、元々動いてなかったんですね。AzurePipelinesの除外パス。
やっぱりちゃんとテストしないと駄目ですね。

[berryzplus]

良さそうなのでDraft外します。

[berryzplus]

れびゅーありがとうございます。
マージしてAzure Pipelinesのスケジュールを止めときます。

[berryzplus]

Azure Pipelinesのスケジュールが見つからなかったので、設定削除の件は放置します。

[berryzplus]

マージの静的解析が出たので報告。
https://github.com/sakura-editor/sakura/runs/1682778477

バグ件数3875件（＋3545件）
増加したバグは、ほぼHTMLの書きっぷりがHTML3.2以前だから。

クオリティゲートがFailedになっている理由は、
直近30日間に修正されたコードのカバレッジが8割を下回っているから。

[ghost]

このPRで閉じられるissueって、 #1486 「GHAの構成がおかしい」で合っていますか？
既存のワークフローは一切変更されていないように見えるのですが。