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

Have a better way to auto-update dependencies #15694

Closed
Piedone opened this issue Apr 9, 2024 · 9 comments · Fixed by #16305
Closed

Have a better way to auto-update dependencies #15694

Piedone opened this issue Apr 9, 2024 · 9 comments · Fixed by #16305
Milestone

Comments

@Piedone
Copy link
Member

Piedone commented Apr 9, 2024

Is your feature request related to a problem? Please describe.

Dependabot is currently enabled and it updates some dependencies automatically. However, this is not ideal, see from here.

Describe the solution you'd like

Perhaps we should disable Dependabot, or use Renovate.

See this issue with some ideas: Lombiq/Open-Source-Orchard-Core-Extensions#703

Describe alternatives you've considered

Just disabling Dependabot.

@hishamco
Copy link
Member

hishamco commented Apr 9, 2024

This could be done automatically but again, there's no testing well be involved, right :)

@Piedone
Copy link
Member Author

Piedone commented Apr 9, 2024

Yep, and while the update itself can be automated (which is I think only worthwhile in itself if it's about some urgent security update), this can happen only up to a point of opening a PR. Then somebody still needs to check that, and as Antoine told under the linked thread, Gulp also run.

@Skrypt
Copy link
Contributor

Skrypt commented Apr 9, 2024

I don't like the fact that we update these without really testing them manually. It often breaks feature because we don't have proper functional tests. I wouldn't mind if we had Playwright first so that we can plan having basic tests for some of them that are really important.

@Piedone
Copy link
Member Author

Piedone commented Apr 9, 2024

We should test them manually. When I review such a PR, I always also test it, but of course, the author should do this foremost, since that's most of the work usually anyway. I also ask for that.

I created a UI test suite for this before: #11194 But since then the only thing we could progress on was "maybe let's use Playwright". We really need a UI test suite, because we had dozens of regressions lately that would basically all be caught by just a few UI tests.

@sebastienros
Copy link
Member

Why not try to update the https://github.com/OrchardCMS/OrchardCore/blob/main/.github/dependabot.yml file to include these libraries we currently update quite often. I don't see our Renovate would be better (haven't checked) if we configured dependabot correctly.

Or add dependabot rules when we have some minimal functional test for the feature it represents? Example "phone number parser", or "markdig"...

@sebastienros sebastienros added this to the 1.x milestone Apr 11, 2024
@Skrypt
Copy link
Contributor

Skrypt commented Apr 11, 2024

I don't mind. But as I said. If you break something by updating these then take responsibility into fixing it afterward too. That's the part that this dependabot will not do. I personnally just don't like it.

It should report only when there is a security issue and that something needs to be updated.

Else, generally when I need to work on enhancing a feature then I'm trying if I can upgrade dependencies. This way, when I create a PR ; it becomes about a enhancement that adds that extra value. For me it doesn't work the other way around ...

@BenedekFarkas
Copy link
Member

BTW there are 16 Dependabot security alerts at the moment and 9 of them has a PR that was closed. If those packages have been updated in another PR that adds other (necessary) changes, then the corresponding alerts should be marked as resolved with a link to the PR.

@Piedone
Copy link
Member Author

Piedone commented Apr 25, 2024

Those alerts should auto-resolve themselves if a fix is in main. So that indicates that those weren't fixed in other PRs.

@BenedekFarkas
Copy link
Member

OK, then that's not very ideal (I mean that they were closed without action).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants