Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: cargo-denyのadvisoriesだけcronでの実行にする #893

Merged
merged 5 commits into from
Dec 23, 2024

Conversation

qryxip
Copy link
Member

@qryxip qryxip commented Dec 11, 2024

内容

cargo-denyワークフローをlicensesauditに分離し、後者を毎日のcron実行とする。

関連 Issue

その他

RustSecへの登録によって「何もしていないのにCIが落ちた」ケースが最近増えてきたため、ENGINEのようにcronだけで動かす方が妥当かなと思った次第です。
(もしかしたらENGINEと同様、Discordへの送信にした方がよいかもしれない)

@qryxip qryxip requested a review from Hiroshiba December 11, 2024 19:30
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちょっと目的を整理しておけると!
目的次第ですが、どっちもcron実行・mainブランチで実行にしても良いのかも?

たしかcargo-denyをmainブランチでのテストにしてた理由は、ファイルを更新するのは大変なので、コミッターでなくメンテナが更新すれば良いようにしたかったからだった記憶。
mainブランチのテストはたまに落ちて良いという設計っぽみ。

今回のPRはadvisoriesのみmainブランチから外してcronにし、ほかはmainブランチで動かす感じですよね。
提案としては、まあどっちもcron・mainブランチで動かすようにすればよいのかな、みたいな。
(だとしたらファイルを分けなくて済むので)

追記:
あいや違うか、advisoriesだけはauditで最悪スルーしてリリースできるけど、ほかは無視するとリリースできないみたいな・・・?
だとしたらなぜファイルを分けてるかのメモコメントというか、各ファイルが何を担当しているのかわかるようにするのが良さそう!

(audit(監査)だとなぜadvisoriesだけしているのかわからない)

@qryxip
Copy link
Member Author

qryxip commented Dec 12, 2024

どちらかというと、licenses (advisories以外のcargo-deny)はライブラリを追加したときに反応するので、こちらにはcron実行するメリットは無いと思った次第です。なので役割的には分離した方がよいかなと。

一方audit (advisoriesのみのcargo-deny)はライブラリの追加に関係無く反応するのが主になると思ってます。
(ライブラリを新規追加したときにそれがヤンクされていたり、脆弱性が報告されていたりのケースもまあ有り得なくはないけど、レアケースと考えてよさそう)

advisoriesをリリースで無視してよいかは、内容によりそう。極端な話「作者のアカウントが乗っ取られました。この最新バージョンはマルウェアです」は駄目だし…

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 15, 2024

なぁるほどです。

ちなみに「advisories以外のcargo-deny」は結構難しいあのdeny.tomlの編集が不要・・・だったりはしないでしょうか。
というのもworkflowファイルがだいぶ多くなっているので、可能なら小分けにしたくないなぁと・・・。
でも無理なら仕方なさそう!

ジャストアイデアでやるべきかはわかりませんが、「advisories以外のcargo-deny」をtest.ymlに移しちゃって、mainブランチ以外では動かないようにするのもあり・・・かも・・・?

@qryxip
Copy link
Member Author

qryxip commented Dec 18, 2024

ちなみに「advisories以外のcargo-deny」は結構難しいあのdeny.tomlの編集が不要・・・だったりはしないでしょうか。

逆ですね… advisoriesだけ基本的にdeny.tomlの編集が不要です。
(脆弱性情報について無視の対応を取る場合は必要)

というのもworkflowファイルがだいぶ多くなっているので、可能なら小分けにしたくないなぁと・・・。

個人的にはtestに詰め込みすぎなので、workflowに関してはむしろ #382 のように小分けにした方がいいんじゃないかなと思っています。減らすことの恩恵があるとすればworkflow数じゃなくてjob数なんじゃないかと。

@Hiroshiba
Copy link
Member

なるほどです!
ファイル数を減らした方がメンテしやすいですが、確かにjob数を減らす観点だと小分けにするのが有利ですね!

エンジンの方はファイル数削減がうまくいったので思いついた次第でした。
でもコアはテストの種類が桁違いになってくるので、job数削減のためにファイルが増えてくるのはやむなくなりそう。
github workflowファイルの構文が変わってこのあたり融通効くようになることに期待。

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ほぼLGTMです!

コメントドキュメント足しの提案をしました!

.github/workflows/audit.yml Show resolved Hide resolved
.github/workflows/licenses.yml Outdated Show resolved Hide resolved
@qryxip qryxip requested a review from Hiroshiba December 19, 2024 20:55
Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!!

Ⅰ箇所コメントしていますが、変更するしないはどちらでも良さそう!
メンテナンス性が上がって嬉しいです!

@qryxip qryxip merged commit 2ac59f4 into VOICEVOX:main Dec 23, 2024
29 checks passed
@qryxip
Copy link
Member Author

qryxip commented Dec 23, 2024

@qryxip
Copy link
Member Author

qryxip commented Dec 23, 2024

↑ あ、cargo binstallの部分がシェルコマンドか。ならshellが無いとだった。

qryxip added a commit to qryxip/voicevox_core that referenced this pull request Dec 23, 2024
qryxip added a commit that referenced this pull request Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants