search

Tokyo-Metro-Gov / covid19

東京都 新型コロナウイルス感染症対策サイト / Tokyo COVID-19 Task Force website
https://stopcovid19.metro.tokyo.lg.jp/
MIT License
6.26k stars 1.97k forks source link

Renovate（依存ライブラリ更新）の運用方法を考える #5142

Closed kaizumaki closed 2 years ago

kaizumaki commented 4 years ago

5068 でRenovateを導入しました。現在は試験運用中で、すべてのアップデートにおいてオートマージしないようにしています。スケジューリングも設定していません。

懸案事項

検討事項

運用体制も含め、みなさんの意見を伺いたいです。

kaizumaki commented 4 years ago

どなたかわかる方！以下のアップデートは適用しても大丈夫でしょうか？

shgtkshruch commented 4 years ago

nuxt-i18n は Netlify の Deploy Preview で以下の点で動作確認できたので、マージして良いと思います。

eslint はローカルで yarn lint を実行してエラーが無かったので、マージして良いと思います。 （GitHub Actions で動かしている Reviewdog が適切に動けば、そこでチェックできると思います）

こういったテスト項目を CI やテストでカバーできると、検証のコストを下げられると思います。

kaizumaki commented 4 years ago

@shgtkshruch おお！ありがとうございます！ 🙏

こういったテスト項目を CI やテストでカバーできると、検証のコストを下げられると思います。

そうなんですよね。ぜひ自動化したいところです。テストの導入も考えた方がよさそうですね。

shgtkshruch commented 4 years ago

Renovateのbotが立てたPRをひとつひとつ検証する作業が結構たいへんである

CI の改善やテストなど色々観点はあると思いますが、まずは package.json で管理するライブラリを減らすのはどうでしょうか？ nodemon や rimraf などすでに利用していないライブラリがいくつかあるので、こういったパッケージを削除するとアップデートの PR 数を減らせると思います。

また、vue-meta や TypeScript、ts-loader など直接依存していないライブラリも package.json に追加されているように思います。 管理できる & 管理するポリシーであれば良いと思いますが、今の体制でアップデートを検証するのは負担も大きいように思うので、必要があればこれらのライブラリの扱いも検討すると良いのではないでしょうか。 （僕も見れれば良いのですが、なかなか Watch できずに申し訳ないです…）

パッチアップデートをどうするか（自動or手動）

個人的には、自動テストがない状態での自動マージはちょっと心配ですが、リリース前に Staging で表示確認をするなどのフローがあれば良いかもしれません。 続けられるかどうか & 自信を持ってできるかどうかが大事だと思います。

kaizumaki commented 4 years ago

dayjsのパッチアップデート #5131 は、previewで「1月1日」誤表示 #5150 を見落としてしまったんですよね。そういうこともあって、パッチアップデートも慎重になっています。目視確認のたいへんさと限界を感じています。

たしかに、まずはpackage.jsonに書かれている依存ライブラリをいかに減らすかを考えた方がよさそうですね。

mcdmaster commented 4 years ago

そこで package.json の "resolutions" ブロックの出番です(笑) https://classic.yarnpkg.com/ja/docs/selective-version-resolutions/

上記説明にも書かれているように、

といった依存関係のあるモジュールを "resolutions" に切り出しておけば、そんなに至近でない、でもやがてやってくるであろうメンテナンスの必要性を意識させつつも（リスクの潜在化）、日々のアップデートのような緊急性の外に置くことができます。

ちなみに、私は個人的に下記のような "resolutions" ブロックを使っています。これらは、よほどのことがない限りセキュリティ脆弱性に伴うアップデートはないだろうというものを切り出しています。 あるいは、RC 版と現行プロダクション版の依存関係を併存させたりもしています。 ここにあるように、かなり非特定的なバージョン記法も許されます

  "resolutions": {
    "@vue/component-compiler/vue": "^3.0.0-rc.5",
    "@vue/server-renderer/vue": "^3.0.0-rc.5",
    "chokidar": "3.x",
    "core-js": "3.x",
    "css": "3.x",
    "source-map-resolve": "0.6.x",
    "vue-server-renderer/vue": "2.x",
    "vue-template-compiler/vue": "2.x",
    "webpack": "4.x || 5.x"
  }
goki90210 commented 4 years ago

たしかに、まずはpackage.jsonに書かれている依存ライブラリをいかに減らすか

ソースから直接使用しているものはpackage.jsonから削除してはいけません。 ただし、依存関係を正しく理解することがどこかで担保されている場合はこの限りではありません。 （yarn.lockを見ろはナシです。）

上記についてはNon-Null Assertion Operatorについても同様です。

なんの説明もなしにこれらが行われると、保守性が著しく損なわれます。

kaizumaki commented 4 years ago

@goki90210 なるほど、「利用していないライブラリがあるのではないか」という文脈でそのように書いたのですが、今のところpackage.json内に利用していないライブラリは混ざっていない、ということですかね？

kaizumaki commented 4 years ago

RenovateのPRで、nodeのマイナーアップデート #5115 についてはずっとそのままにしているのですが、これはどういう判断でアップデートしてよし、となるのでしょう...？

goki90210 commented 4 years ago

RenovateのPRで、nodeのマイナーアップデート #5115 についてはずっとそのままにしているのですが、これはどういう判断でアップデートしてよし、となるのでしょう...？

@kaizumaki 本来は、自動でテストを実行するスクリプトがあってアップデートしてもテスト結果に変化はないことを検証/確認するというのが筋なのですが（各パッケージのリリースもそいう運用になっているはず）、このプロジェクトはテストケースは作成されていませんし、作成されていたとしても漏れる場合もありますし、判断は難しいですね。

「利用していないライブラリがあるのではないか」

dependencies

devDependencies

理由の記載がないものはpackage.jsonに載せている理由が私には不明です。 （以前も書きましたが、特にjest, babel-jest, vue-jest）

mcdmaster commented 4 years ago

私からもコメントさせてください。特に、 @goki90210 さんが「不明」と指摘しているものについてです。

わたしは、これっくらいでカンベンしておきましょうか。 jest 系は、よくディペンデンシーで warning が出るので、私も外せるものなら外してしまいたいと思っています

kaizumaki commented 4 years ago

本来は、自動でテストを実行するスクリプトがあってアップデートしてもテスト結果に変化はないことを検証/確認するというのが筋なのですが（各パッケージのリリースもそいう運用になっているはず）、このプロジェクトはテストケースは作成されていませんし、サウ製されていたとしても漏れる場合もありますし、判断は難しいですね。

やはり難しいですか...。それでも自動テストはあったほうがいいように思いました。 素朴な疑問なのですが、webサイトの自動テストって、具体的にどういうものなんでしょう...？

goki90210 commented 4 years ago

@mcdmaster

  • @nuxtjs/pwa ： nuxt.config.tsで使用している（devDependenciesでもいい気がするが不明）：PWA 用なんで、サーバとかならずしも密に連携しないこともある（回線切れなどのときの）使いっぷりも考えると devDependencies でもいいかもですね
  • @mdi/font → 不明：サイドメニューアイコンのフォント（Material Designs）ですね。必須です
  • @nuxtjs/google-analytics ：アクセスログ取得用です。必須
  • @types/chart.js：Dependencies 側にある chart.js の export やなんかを含んでいます。必須

承知しました。（これらはないとビルドが通らない）

  • @babel/core → 不明：core-js を動かすために必須のモジュールです

core-jsだけではなく、他のライブラリでも依存しているのは承知していますが、 それだけであれば書かなくても良いと思います。バージョンを指定したい意図があるということですか？

jest 系は、よくディペンデンシーで warning が出るので、私も外せるものなら外してしまいたいと思っています

今後jestでテストの自動化を行わないのであればという条件付きですね。 （最初に作成された方はテストの自動化を念頭に置いていたのではないかと考えています。気のせいでしょうか？）

mcdmaster commented 4 years ago

@goki90210

core-jsだけではなく、他のライブラリでも依存しているのは承知していますが、 それだけであれば書かなくても良いと思います。バージョンを指定したい意図があるということですか？

そのとおりです。 core-js をはじめとする babel 系のライブラリは package.json に複数書きだすばかりでなく、それらのバージョンを揃えるという条件があります。 それを説明したドキュメントの場所を失念したので、見つけ次第本コメントのアップデートとして Web URL を貼っておきますね

kaizumaki commented 2 years ago

現時点でRenovateの運用が大体まわっているので、こちらのissueはひとまずクローズします。