-
Notifications
You must be signed in to change notification settings - Fork 29
[code-infra] Shareable renovate preset #391
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
d25d8ea to
cdc2d84
Compare
|
I think sharing via npm package is deprecated:
I was thinking, maybe we can add a top-level renovate folder that holds all presets? |
I saw this as well. So we can use the http url format ( |
cdc2d84 to
0db2481
Compare
|
I was thinking |
c891fab to
3f44180
Compare
6b0863d to
1778d65
Compare
| "prConcurrentLimit": 30, | ||
| "prHourlyLimit": 0, | ||
| "rangeStrategy": "bump", | ||
| "schedule": "on sunday before 6:00am", |
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.
Having all projects build their PRs roughly at the same time can put more stress on the CI and possibly lead to timeouts. What do you think about having each project define its schedule?
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.
Its what there was in all the repos. Downstream repos can still override this value if required. When not provided, this will be the default.
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.
Probably overkill, but if we want to control this value from the central mui-public repo we could create a schedule-<repo> preset for each of the repositories.
4e97ab5 to
4a971db
Compare
renovate/default.json
Outdated
| "matchPackageNames": ["vite", "@vitejs/**", "/vitest/", "esbuild", "^vitest$", "^@vitest/"] | ||
| }, | ||
| { | ||
| "automerge": true, |
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.
This doesn't work as well as it could as all PRs require explicit approval. I added it in Base UI, but there's not much value TBH
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.
On X we have a top rule to automerge any non-overridden dev dependency updates.
It works well-ish, especially when CI is stable. 👌
https://github.com/mui/mui-x/blob/37a677b4d4137c39c0415517144990dede8aadee/renovate.json#L12-L16
4a971db to
a2f7d1a
Compare
| "groupName": "Infra packages", | ||
| "matchPackageNames": ["@mui/internal-*", "@mui/docs"], |
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.
I wonder how this will combine with other wildcard patterns.
Do you know how it will work if we inherit this config and have a @mui/* group in the specific package file?
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.
Link - docs.renovatebot.com/configuration-options#packagerules
If multiple rules match a dependency, configurations from matching rules will be merged together. The order of rules matters, because later rules may override configuration options from earlier ones, if they both specify the same option.
So in this particular case, followTag will also have to be added in your @mui/* group so that it overrides the value from the base config.
| "matchPackageNames": ["@types/node", "node", "cimg/node", "actions/setup-node"], | ||
| "enabled": false |
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.
I'm having second thoughts about this.
IMHO, @types/node should be updated within a specified major.
Same for cimg/node and actions/setup-node. Especially the latter, since it does not directly relate to the Node package version. 🤔
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.
Posted reply to the wrong comment. It's for your previous 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.
I recently proposed to change this in core repo. not sure how well it will work though. not all these environments necessarily support the same minor versions.
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.
I agree that actions/setup-node should not be in the same group because its version can be updated independently of others. So I have removed it from the group in this PR.
not all these environments necessarily support the same minor versions
It should be fine as long as we specify only a major version in allowedVersions intead of pinning to a specific node version. WDYT ?
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.
renovate should configure the node version of actions/setup-node, I think it belongs in this group. the action itself can be latest version. we may have to specify which package manager we target exactly.
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.
Should it?
I think we only specify the major and AFAIK, we manually decide when we want to bump the major. 🤔
not all these environments necessarily support the same minor versions
It should be fine as long as we specify only a major version in allowedVersions intead of pinning to a specific node version. WDYT ?
Yes, renovate group could specify that it allows "<23".
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.
Should it?
if I interprete their docs correctly, yes. that's why I made that change, to test whether/how that works. their docs tend to be a bit cryptic sometimes
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.
@Janpot It is enabled: false for this group. So will it still bump the version os actions/setup-node action while not bumping node-version param. I don't have a lot of clarity based on the docs.
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.
Yes, I want to enable it again so we update node versions everywhere we use node and sync the major version through renovate. the group:nodeJs seems to be able to do what we want. Perhaps we should use it, or start from it? I think what could missing on our side is specifying the datasource. I suppose by narrowing it to docker and node-version it excludes updating the github action?
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.
e.g. The following could work:
{
"extends": ["group:node"],
"packageRules": [
{
"matchPackageNames": ["node"],
"matchDatasources": ["node", "docker"],
"allowedVersions": "22.x",
"versionConstraint": "22.x"
}
]
}|
Make sure to run config validator before merging this. |
71271fd to
9cec825
Compare
9cec825 to
a23ca77
Compare
a23ca77 to
30a1bd3
Compare
| { | ||
| "groupName": "core-js", | ||
| "matchPackageNames": ["core-js"], | ||
| "allowedVersions": "< 2.0.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.
Aside, do you know why we do this? I think we should document this somewhere and figure out if it's still relevant.
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.
I personally don't know. I saw this in all the repo, so copied here. Maybe @oliviertassinari or @mnajdova do ?
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.
No idea, it's here since the beginning of time :D mui/material-ui#27003
renovate/default.json
Outdated
| "react-is", | ||
| "@types/react", | ||
| "@types/react-dom", | ||
| "@types/react-is" |
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.
I believe we also use use-sync-external-store in some places. It's also maintained by the react team.
renovate/default.json
Outdated
| }, | ||
| { | ||
| "groupName": "Playwright", | ||
| "matchPackageNames": ["@playwright/test", "mcr.microsoft.com/playwright"] |
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.
I know in some places we only use @playwright/test but perhaps we should widen to include @playwright/* and playwright?
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.
I checked all repos and found that we only explicitly depend on @playwright/test and no other playwright package. But agree that it wouldn't hurt
| "groupName": "node", | ||
| "matchDatasources": ["docker", "node-version"], | ||
| "matchPackageNames": ["@types/node", "node", "cimg/node", "actions/setup-node"], | ||
| "enabled": false |
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.
I believe we could mandate a major instead of disabling?
| "enabled": false | |
| "allowedVersions": "22" |
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.
Before that, we'll have to make sure all repos actually are using this version
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.
If not we can also create presets for each node version we want to support, so on repo side they can do
{
"extends": ["github>mui/mui-public//renovate/default", "github>mui/mui-public//renovate/node-20"]
}
Janpot
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.
This seems mostly fine, I'd like to figure out if we can steer all node versioning from the renovate config if possible. Also ok for me if you want to look into it in separate PR.
|
Yeah. I'll tackle the node versioning in a separate PR. Lets first bring all repos to node 22. |
This config will be extended in the downstream repos using -
{ "extends": ["github>mui/mui-public//renovate/default"] }See - https://docs.renovatebot.com/config-presets/#github
Companion PRs -