8000 ビルド周りのGithub Workflowのリファクタリング by Hiroshiba · Pull Request #636 · VOICEVOX/voicevox_core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 11, 2023
Merged

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

merged 1 commit into from
Oct 11, 2023

Conversation

Hiroshiba
Copy link
Member

内容

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

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

関連 Issue

その他

の前作業です。

Hiroshiba
Hiroshiba commented Oct 10, 2023
View reviewed changes
.github/workflows/build_and_deploy.yml
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.

多分大丈夫だと思います

qryxip
qryxip reviewed Oct 11, 2023
View reviewed changes
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.inputsはinputsにできるので短くした

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
@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

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

@Hiroshiba Hiroshiba mentioned this pull request Oct 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sevenc-nanashi sevenc-nanashi sevenc-nanashi left review comments

@qryxip qryxip qryxip approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Hiroshiba @qryxip @sevenc-nanashi
0