Conversation
|
Preview (prod) → https://5005-prod.traq-preview.trapti.tech/ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5005 +/- ##
==========================================
+ Coverage 60.81% 60.98% +0.16%
==========================================
Files 101 101
Lines 2973 2973
Branches 616 616
==========================================
+ Hits 1808 1813 +5
+ Misses 1071 1063 -8
- Partials 94 97 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0b1fa61 to
2ff19c4
Compare
There was a problem hiding this comment.
Pull request overview
This PR removes config.js from the build process, requiring it to be mounted externally when deploying with Docker. The change introduces TypeScript type definitions and JSON schema for configuration validation, and adds automated schema generation via GitHub Actions.
- Removed
public/config.jsfrom the repository and excluded it from build artifacts via a custom Vite plugin - Added
config.d.tsTypeScript definitions andconfig.schema.jsonfor configuration validation - Updated Docker deployment to require external mounting of
config.jswith validation in the startup script
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Added Rollup plugin to exclude config.js from build output |
| tsconfig.json | Included config.d.ts in TypeScript compilation |
| tests/unit/tsconfig.json | Included config.d.ts for unit tests |
| public/config.js | Removed configuration file from repository (143 lines deleted) |
| package.json | Added ts-json-schema-generator dependency for schema generation |
| package-lock.json | Updated lock file with new dependency and transitive dependencies |
| config.schema.json | Added JSON schema for configuration validation (134 lines) |
| config.d.ts | Added TypeScript type definitions for Config interface (118 lines) |
| build/docker/startup.sh | Added validation check to ensure config.js is mounted at startup |
| Dockerfile | Removed config.js from default files as it's now externally injected |
| README.md | Added Docker deployment documentation requiring config.js mounting |
| .github/workflows/generate-schema.yml | Added automated workflow to regenerate schema from TypeScript definitions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ecf7ce5 to
768d51d
Compare
768d51d to
c5a9e4c
Compare
| ENV THEME_COLOR=#0D67EA | ||
|
|
||
| # 設定上書き処理用に、.brを消して、元の設定を別のディレクトリに保存しておく | ||
| # config.js は外部から注入されるため除外 |
There was a problem hiding this comment.
[q] これって breaking change はあります?(このままアップデートかけても壊れない?)
というか config.js は消すと fork 先がまずかったりする?もしかして(ちゃんと想像がついてないけど) 特に ex とかで困る可能性があるから、どういう対応が必要になるかが分かるような資料は必要になるかも (理想はそのままアップデートかけてもなにも問題がない状態)
There was a problem hiding this comment.
manifest 側の対応が済んでいれば壊れることはないはずですが、fork は何もしないと確実に動かなくなると思います。
ちょっと考えてみます
|
|
||
| if [ ! -f /app/override/config.js ]; then | ||
| echo "ERROR: config.js is not mounted at /app/override/config.js" | ||
| echo "Please mount config.js when starting the container:" |
There was a problem hiding this comment.
[imo] README.md に誘導する文言入れてあげてほしいかも
| CACHE_KEY=$(md5sum /usr/share/caddy/new-relic.js | cut -d ' ' -f 1) | ||
| mv /usr/share/caddy/new-relic.js /usr/share/caddy/new-relic-$CACHE_KEY.js | ||
| sed -i -e "s/<!-- <script src=\"\/new-relic-{hash}.js\"><\/script> -->/<script src=\"\/new-relic-$CACHE_KEY.js\"><\/script>/" /usr/share/caddy/index.html | ||
| echo "Startup: set up New Relic" |
There was a problem hiding this comment.
なんか format がかかっちゃってる?理由がなければ戻してほしいかも
| on: | ||
| push: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
[imo] できれば master で push する前に PR のタイミングで生成してもらう(自動テストで diff ないかチェックしてあったら落とす) の方が、 master で変なデータが挟まる可能性がなくて良い気がする
|
|
||
| cp "${CONFIG_SOURCE_PATH}" "${CONFIG_DIST_PATH}" | ||
| else | ||
| echo "config.js not found, downloading from manifest repository..." |
There was a problem hiding this comment.
[q] これ echo で出力する理由ってある? なんか 気持ち的には debug レベルの出力( = 普通はなくていい)な気がしちゃう
There was a problem hiding this comment.
手癖で書いてしまいました
私もいらないと思います
| @@ -0,0 +1,13 @@ | |||
| { | |||
There was a problem hiding this comment.
fallback よりなんか default config として使えるといいかも
There was a problem hiding this comment.
たしかにです
薄いほうがいいとは思うのですが、どうしましょう……?
There was a problem hiding this comment.
今の内容で default config を作ってしまえば良いと思います!元々のゴールは conflict 防止なので、別に最悪そこまで薄くなくても良い
ras0q
left a comment
There was a problem hiding this comment.
issue立てて全然動向を終えてなくてすみません。
問題が複雑になっていそうな気がします。
現状の public/config.js ($schema付きJSONでいいのは間違いないが時すでに遅し) をどこでも動くようにfallbackのものにして、各環境でDockerでマウント or フォークで直接書き換えだけだと足りないですかね?
改めて考えるとたしかにそうですね、なんか勘違いしていた気がします |
概要
なぜこの PR を入れたいのか
closes: #4796