-
Notifications
You must be signed in to change notification settings - Fork 2
feat: zipのキャッシュ機構を追加 #40
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
Conversation
scripts/common.ts
Outdated
| // キャッシュを保存するリポジトリ | ||
| export const cacheRepo = "sevenc-nanashi/voicevox-preview-pages"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
メモ:これをvoicevox/preview-pagesに向ける
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あとはこれですかね!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds a cache mechanism for zipping and storing preview-pages artifacts via a dedicated GitHub Release.
- Introduces a new
uploadCachesscript to create or reuse a cache release and upload.zipartifacts. - Extends
DownloadDatawithrunIdandcachedflags, updates collection logic to check and store cached artifacts. - Updates CI workflow and npm scripts to run the cache upload step.
Reviewed Changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/uploadCaches.ts | New script for checking/creating a cache release and uploading zips |
| scripts/constants.ts | Added runId and cached to DownloadData |
| scripts/common.ts | Added cache constants, helpers (getCachedArtifact, createCacheFileName) |
| scripts/collectArtifacts.ts | Integrated cache lookup/download, split download vs. extract steps, metadata writing |
| package.json | Added run:upload-caches script |
| .github/workflows/update_pages.yml | Added “Upload Caches” step |
Comments suppressed due to low confidence (1)
scripts/collectArtifacts.ts:1
- The new
extractArtifactimplementation usesunzip.Extract, but there's no import for anunzipmodule. Add the proper import (for example,import unzip from 'unzip-stream'or similar) to enable extraction.
import fs from "node:fs/promises";
Hiroshiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
いろいろコメントしてみましたが、まあこだわりまくる必要もないと思うので、バグっぽいとこ確認して気が向いたらリファクタリングに挑戦いただければくらいの気持ちです!!
Co-authored-by: Copilot <[email protected]>
|
んー、skipDownloadの時にjsonを書き込むかは若干悩んでます。jsonだけあってzipがない状況がちょっと変な気がして… |
|
あ、別にバグではないのであればjson書き込まなくて良い気がします!! (今思えば今回のPRは目標の設計が定められてなくて、何が正解なのかわかってなかったかもです。) 今downloads.json?とzipInfoのjsonと情報源が2つありそうですかね? 情報源が2つあるし、しかもuploadArtifacts内は「jsonファイルが有るかどうか」で処理を分けていて、ちょっと複雑かもと感じました! |
|
そもそもskipDownloadはテスト用にダウンロードURL取得までして終了するときのオプションですね。その時はjsonを書き込まないって感じです。 |
|
な、なるほど・・・! たぶん
というのがわかる変数名にしておくと良さそうかも。 |
|
あ、実装はもうできてるということがわかったので、課題ありそうなところをとりあえずFIXMEコメントにしてマージする手もあると思います! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
色々コメント書いてますが、LGTM!!
ぶっちゃけ空中配線感がそこそこ・・・というかまぁぶっちゃけだいぶ追加されてる感じがして、多分将来見直した時にこのプルリクエストを作った時の自分に切れそうになるんだろうなと感じています!
気になったのはキャッシュキー周りの扱いで、固定にしとかなくちゃいけないものと別に固定じゃなくてもいいものがどれがどれかわかんなくて、おそらく別の人が将来変更する時はなかなか気を使うと思います。
それでも別に一旦マージしちゃって良いと思います!
今気づいている設計上の課題点コメントを書きまくって(ちなみにFIXMEコメントやTODOコメントやNOTEコメント書きまくるのは将来変更する時の把握漏れ防止になるのでかなり重要だと思ってます)、将来切れてる自分が素早く状況を理解できるようにしておくぐらいでもいいかなと。
scripts/collectArtifacts.ts
Outdated
| console.log(`Usage: collectArtifacts.ts [--skipDownload]`); | ||
| console.log( | ||
| `--skipDownload: ダウンロードURLの取得までを行い、それ以降のダウンロードや展開をスキップします。`, | ||
| ); | ||
| process.exit(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(あまり強くないコメントです)
ここ、コードをAI君に読んでもらってる感じずっと混乱してるんでまだちょっと分かりにくいんだと思います。
前者の取得だけするっていうところの方を引数名にすればさらに分かりやすくなるかも?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiroshiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ほぼLGTMです!!
いくつか提案してますが、良さそうなのだけ取り込む感じで良さそう!
残ってるのはfixmeなりtodoコメントにしてマージでも良さそう!
scripts/collectArtifacts.ts
Outdated
| console.log(`Usage: collectArtifacts.ts [--skipDownload]`); | ||
| console.log( | ||
| `--skipDownload: ダウンロードURLの取得までを行い、それ以降のダウンロードや展開をスキップします。`, | ||
| ); | ||
| process.exit(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hiroshiba
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!
| if (!targetRepo) { | ||
| throw new Error(`Unknown repo key: ${key}`); | ||
| } | ||
| const [owner, repo] = targetRepo.repo.split("/"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここはどうしようもないような?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、string.split("/")をparseRepo(string)に関数化したんだと思ってました!
まあロジックの分散になってるのでたぶん関数化したほうが良さそうではある
must / should / want / can の wantからshould寄りくらいかなと
scripts/common.ts
Outdated
| // キャッシュを保存するリポジトリ | ||
| export const cacheRepo = "sevenc-nanashi/voicevox-preview-pages"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あとはこれですかね!
|
あとは変更してマージボタン押すか変更せずマージボタン押すかで大丈夫だと思います! |
|
マージします。 |

内容
キャッシュ機構を追加します。
関連 Issue
スクリーンショット・動画など
(なし)
その他
(なし)