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

ビルド周りのGithub Workflowのリファクタリング #636

Merged

Conversation

Hiroshiba
Copy link
Member

内容

ビルド周りのgithubワークフローで、若干統一性がなかったのといくつか気になった点があったのでファクタリングしてみました。

  • github.event.inputsinputsにできるので短くした
  • ASSET_NAMEをステップ動的に定義するのではなく静的に定義できるようにした
    • VSCodeだとステップ内で定義された環境変数は認識されなくてワーニングが出る
  • "contains(matrix.target, 'android')"がエラーになっていたので直した
  • SKIP_UPLOADING_RELEASE_ASSETはもう存在しないのでコード中から消した
  • env.VERSION != '0.0.0'はリリース判定に使わなくなったので変更した

関連 Issue

その他

の前作業です。

Comment on lines -295 to +298
if: "contains(matrix.target, 'android')"
if: contains(matrix.target, 'android')
Copy link
Member Author

Choose a reason for hiding this comment

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

@sevenc-nanashi ここってダブルコードで囲わないとバグる感じでしょうか 👀
(VSCodeでエラーになっていたから修正した形ですが、念のためにお聞きしている感じです)

Copy link
Member

Choose a reason for hiding this comment

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

多分大丈夫だと思います

Copy link
Member

@qryxip qryxip left a comment

Choose a reason for hiding this comment

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

一点だけ疑問が。

github.event.inputsinputsにできるので短くした

inputsって、github.event.inputs'true'とかとは違って真性のbool値が入ってくるんじゃないんでしたっけ? そこは大丈夫でしたでしょうか。

@Hiroshiba
Copy link
Member Author

@qryxip 正直ちょっと若干自信がないのですが、少なくとも今回の場合は大丈夫そうです!

というのも今回はinputs.ほげ'true'と比較しているので、中身が文字列であろうとbooleanであろうと文字列にキャストされて比較されると思います。
https://docs.github.com/ja/actions/learn-github-actions/expressions#contains
== 'true'を使っている限りではどっちでも安全なはず。(これが一番信頼できるはず)

ちなみにinputs(正式にはinputsコンテキスト)はgithub.event.inputsを又渡ししているだけだと思うのですが、github.event.inputsでstringだったboolean値がinputsでちゃんとしたbooleanになるのかはドキュメントに書いてませんでした・・・。

@Hiroshiba
Copy link
Member Author

レビューありがとうございます!マージします!

@Hiroshiba Hiroshiba merged commit 5359c7f into VOICEVOX:main Oct 11, 2023
35 checks passed
@Hiroshiba Hiroshiba deleted the GithubWorkflowのリファクタリング branch October 11, 2023 18:14
@Hiroshiba
Copy link
Member Author

というのも今回はinputs.ほげ'true'と比較しているので、中身が文字列であろうとbooleanであろうと文字列にキャストされて比較されると思います。 docs.github.com/ja/actions/learn-github-actions/expressions#contains == 'true'を使っている限りではどっちでも安全なはず。(これが一番信頼できるはず)

すみません、これ完全に間違えてました。。。。。。。。。。型変換はされませんでした・・・・。
そしてgithub.event.inputsで文字列だったbooleanは、inputsでちゃんとしたbooleanになるっぽいです・・・。
https://github.com/Hiroshiba/test_actions/actions/runs/6486562288/job/17614989125

なので今のコードはバグっている状態になりそうです。
修正プルリクエストを投げます・・・。

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.

3 participants