-
Notifications
You must be signed in to change notification settings - Fork 22
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
Reconfigure/disable renovate #1145
Comments
Yeah, I'm sorry about that, it shouldn't be that way. It was correctly updating dependencies in the background and grouping them into one PR. Then I disabled CircleCI, and it opened the floodgates. The Renovate setup wasn't the one I anticipated so I didn't see that coming. I assumed it was doing silent updates on branches based on master, while it is actually doing PR updates on I can't really disable Renovate completely as I kinda need it running to configure/debug it. All I can suggest is muting the notifications for this repo while I work on the fix 🙏 |
Renovate should be going more smoothly now. It should be back on its initial configuration of creating PRs to merge on But I'd like to discuss more broadly about how you (@Haroenv and @MartinKolarik, and even @bodinsamuel if you'd like to chime in) think we should handle dependency updates. I'm coming from a background where I try to automate dependency updates as much as I can. If it does not break the lint/test/build process, then I usually let Renovate auto-merge any minor or patch upgrade (I keep any major update under manual review). The rationale is that when I need to upgrade a package (and it's a when, not an if IMO), I don't have a very hard migration from 5 major versions ago where it's hard to pinpoint exactly what fails (because everything fails at the same time). On this project though, I've come to realize we have taken a more "if it ain't broken, don't fix it" approach with some packages (as we've seen with the Whatever we decide to do, I think we should be a bit more explicit about our thought process, and write it down. At least discuss it here between us, maybe write some guidelines in the documentation and configure renovate accordingly. To ground the discussion a little bit more, what are your thoughts on the two following packages:
You have much more experience with the code here than I have, so I highly respect your opinions on the matter and am looking forward to them Edit: Also, do we really need Dependabot if we have Renovate? |
I will summarise our approach regarding renovate strategy since it might not be clear from the outside world.
Why not merging directly?Because, and especially in this repo, some parts are not tested or hardly testable. So a broken update can enter the The downsidesIt requires at least one human intervention (can be weekly or less often though) Keeping this strategy is up to you, I don't have strong opinion it just served us very well during the years. Regarding the broken updates, I think it's fair to say we (you) need to do something:
I think Dependabot is forced at the Org level, you can't disable it. |
I'm not overly eager to update everything here (especially in an automated way) because the test coverage isn't too great, and if something breaks, the likely solution is a full reindex, which is still slow. Specifically with As for ESM, it's theoretically just an option in the TypeScript config, but realistically, it also impacts production monitoring (APM instrumentation support), running tests/coverage, and some more advanced mocking scenarios. The ecosystem support is fairly good at this point, and I think on this project, the impact should be low, but so is the motivation to do it - all of the Sindre's projects work fine in their older CJS versions, and there is close to zero advantage in running this as ESM at this point.
While I'm in general a fan of using built-in staff, the fetch is very limited compared to |
Summarizing the points above, what I think we can do is:
Did I miss anything? |
(Oh also, I could disable Dependabot. Maybe it will be automatically re-enabled again, we'll see.) |
The security risk is just a ReDos, not applicable in our case. If we'd do anything I'd migrate to what npm uses (a different package if I remember it correctly) or vendor the code |
@Haroenv The Would you had another in mind they could be actually using? |
I don't know what the config options are, but it's extremely spammy in the current form. It would be better if it sent a single PR with all updates periodically or something like that.
The text was updated successfully, but these errors were encountered: