Skip to content
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

[code-infra] Set renovate to automerge devDependencies #13463

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

JCQuintas
Copy link
Member

Currently only set for devDependencies we can increase the automerge coverage to regular deps and the package lock refresh once we are confident in it.

@JCQuintas JCQuintas added the core Infrastructure work going on behind the scenes label Jun 12, 2024
@JCQuintas JCQuintas self-assigned this Jun 12, 2024
@mui-bot
Copy link

mui-bot commented Jun 12, 2024

Deploy preview: https://deploy-preview-13463--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against de62311

@LukasTy
Copy link
Member

LukasTy commented Jun 12, 2024

Checked pending PRs.
This would cause the following to be merged:

If I understand the behavior of this change correctly, I would be wary of introducing it. 🙈

@JCQuintas
Copy link
Member Author

For the testing libs, does it even matter? We would have two different ones, we can just remove them when we want.

For react, why we can't merge it if the pipelines are passing? The website also seems correctly rendered.

It is also possible to ignore react changes though

@LukasTy
Copy link
Member

LukasTy commented Jun 12, 2024

For the testing libs, does it even matter? We would have two different ones, we can just remove them when we want.

But without seeing those PRs, we might not ever even notice these discrepancies and possible improvements/simplifications that could be done. 🤔

For react, why we can't merge it if the pipelines are passing? The website also seems correctly rendered.

It's WIP: #12295 (comment)

@JCQuintas
Copy link
Member Author

If chai/RTL are going to be used directly I would expect them to be in the package.json. Else, we can just remove them. DevDeps do get out of sync a lot and that is normal, I would rather use a tool for that, than waiting for the upgrade tool to open a PR and rely on someone verifying that the dep is still used.

I saw it was blocked by the main repo PR, but why? Would it break development? Would it break for our users?

@LukasTy
Copy link
Member

LukasTy commented Jun 12, 2024

I saw it was blocked by the main repo PR, but why? Would it break development? Would it break for our users?

If the CI is green, then probably just out of optimization reasons—so as not to introduce duplicate dependency major versions if they are not necessary.

I would still like to have the option for the following flow: a dependency PR is red, we start looking into it, but we want extra confirmation before allowing it to be merged even if the CI is green (the React bump PR case).
What could be the flow then?
Converting the PR to draft?
Adding an on hold label to prevent auto-merge? 🤔

I would love input from @mui/code-infra on this question. 🙈

@JCQuintas
Copy link
Member Author

JCQuintas commented Jun 12, 2024

I generally don't like arbitrary exceptions on CI. Green is good, red is bad. If green is bad we have to fix so it doesn't happen again.

React is a peer dependency and a dev dependency. The only people affected would be us. I don't see what would be the issue in merging all those PRs you mentioned.

I would expect renovate to completely hands off all PRs where someone has added a commit (though i didnt find docs on the automerge page), so if the pipeline goes red and we add a commit, it should work as you expect.

@michaldudak
Copy link
Member

What could be the flow then?
Converting the PR to draft?
Adding an on hold label to prevent auto-merge? 🤔

I would love input from @mui/code-infra on this question. 🙈

You could add a negative review (request changes) to the PR. Renovate won't merge such PRs automatically.

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Sorry, I forgot about this PR. 🙈
I think we can try experimenting with automerging dev deps excluding major bumps. 👍

I would love to have the option to automerge only when the diff has equal amounts of additions and removals. 🤔 😆

renovate.json Show resolved Hide resolved
@LukasTy LukasTy added the scope: infra Org infrastructure work going on behind the scenes label Nov 6, 2024
@LukasTy LukasTy changed the title [core] Set renovate to automerge devDependencies [infra] Set renovate to automerge devDependencies Nov 6, 2024
Co-authored-by: Lukas Tyla <[email protected]>
Signed-off-by: Jose C Quintas Jr <[email protected]>
@JCQuintas JCQuintas changed the title [infra] Set renovate to automerge devDependencies [code-infra] Set renovate to automerge devDependencies Nov 6, 2024
@JCQuintas JCQuintas added scope: code-infra Specific to the core-infra product and removed scope: infra Org infrastructure work going on behind the scenes labels Nov 6, 2024
@JCQuintas
Copy link
Member Author

Sorry, I forgot about this PR. 🙈 I think we can try experimenting with automerging dev deps excluding major bumps. 👍

😆

I would love to have the option to automerge only when the diff has equal amounts of additions and removals. 🤔 😆

Don't think it is currently possible. Though having different versions should mostly be ok in dev deps.

@JCQuintas JCQuintas enabled auto-merge (squash) November 6, 2024 17:28
@JCQuintas JCQuintas merged commit 1f2fbd8 into mui:master Nov 6, 2024
16 checks passed
@JCQuintas JCQuintas deleted the Set-renovate-to-automerge-devdeps branch November 6, 2024 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes scope: code-infra Specific to the core-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants