Skip to content

validate renovate config file with version matrix#18

Open
Nakano Masaki (namachan10777) wants to merge 2 commits intomainfrom
test/validate-with-matrix
Open

validate renovate config file with version matrix#18
Nakano Masaki (namachan10777) wants to merge 2 commits intomainfrom
test/validate-with-matrix

Conversation

@namachan10777
Copy link
Copy Markdown
Contributor

参考: https://github.com/arkedge/renovate-config/pulls

現状のCIには以下の問題があります

  • arkedge/renovate-configを読むrenovateは複数種類あり得る
    • Mend, self-hostedの2種類、self-hostedの場合動くrenovateのバージョンが必ず最新とは限らない
  • テストツール(特にrenovate-config-validator)の更新を行うPRが大量に発生する

これらの問題を解決するために、package.jsonpnpm-lock.yamlを削除してGitHub Actionでのmatrix strategyでnpm i -gでツールをインストールするようにします。

preset自体は広く使われるものであるので細かいバージョンを指定してそのバージョンでのみ検証を行うのはあまり適切ではありません。また、matrix strategyで検証する以上package.jsonでもバージョンを指定するのはかえって混乱を招きます。
そのためpackage.jsonpnpm-lock.yamlを削除しました。

他への影響

なし(CI部分の変更に留まる)

議論の余地

  • prettierpacakge.jsonで入れてもいいのではないか ?

    • prettierはバージョンが変わるとフォーマット戦略が変わる(そう頻繁に変わるものではなかったように思うが)ので、固定した方が安定して嬉しいかもしれない
    • 単に壊れ出したらformatしなおせば良い。jsonのフォーマット戦略がそうそう変わるとは思えない
  • matrixに設定した3736は適切か?

  • matrixに指定するバージョンは手で更新するので大丈夫か?

    • せめて通知は欲しいような
    • majorバージョンだけなら問題ないし、そもそも人間が関与してバージョンを上げるべきでは

@namachan10777
Copy link
Copy Markdown
Contributor Author

sksat (@sksat) リマインド

Comment on lines +11 to +27
matrix:
renovate_version: [37, 36]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

matrix に設定する Renovate のバージョンについては一旦この方針で行きましょう.(特に major update のような)更新は人間側も強く認識しておくべき,というのはあるので,このバージョンの適切な追従については後で考えましょう.

Comment thread .github/workflows/lint.yml Outdated
- run: pnpm run lint

- name: Install linter
run: npm install -g prettier renovate@${{ matrix.renovate_version }}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

prettier についてはバージョン固定したいですね.もちろんこのリポジトリの性質上存在する場合は json ないし json5 だけがちではあり,実質的にはこれらのフォーマット戦略が変わることはそうそう無いとは思いますが,無いわけではありません.直近でも,tsconfig.json の trailing comma の扱いについてのゴタゴタは記憶に新しいところです(まあ,アレは json であって json でないという特有の問題があったからこそではあるのだけど).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

あと,この内容なら renovate config の lint と prettier の lint(というか format check)は workflow 分けていいと思います.matrix の度に prettier するの無駄だし.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

matrix の度に prettier するの無駄だし.

確かに。job分けます

@namachan10777
Copy link
Copy Markdown
Contributor Author

prettierはpnpm installで入れることにしました。renovateに関してはworkflow内でnpm i -gしてlintをしているのでpackage.jsonからは抜いています。

Comment thread .github/workflows/lint.yml Outdated
steps:
- uses: actions/checkout@v4
- uses: pnpm/action-setup@v2
- uses: actions/setup-node@v4
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

major tag に変えてるのはなぜでしょう?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

書き直したので手癖でやってしまってました。patchまで指定したほうがいいですかね?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

sksat (@sksat) これどうですか

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

major tagに変える必要なかったのでrebaseして変更をコミットから取り除きました

@namachan10777 Nakano Masaki (namachan10777) force-pushed the test/validate-with-matrix branch 2 times, most recently from 82de4f6 to afabdec Compare May 29, 2024 07:08
@namachan10777 Nakano Masaki (namachan10777) force-pushed the test/validate-with-matrix branch 2 times, most recently from 6d15097 to 2011a8b Compare May 29, 2024 07:13
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.

2 participants