search

openequella / openEQUELLA

Core openEQUELLA sources
https://openequella.github.io/
Apache License 2.0
42 stars 44 forks source link

Renovate configuration discussion #2903

Closed HonkingGoose closed 2 years ago

HonkingGoose commented 3 years ago

Thanks @HonkingGoose! 🙇 Opened #2900 to address this, any other deprecated properties you see? Or other improvements that could be made to the configuration?

@ChristianMurphy asked for help/feedback on the current Renovate configuration. I have listed some things to start the discussion. 😄

Automerge devDependencies

I noticed you are trying to setup automerge for all devDependencies:

    {
      "depTypeList": ["devDependencies"],
      "automerge": true
    },

Is this actually working for you? From what I can see, it appears that a maintainer is manually approving and merging them?

Hitting rate limits on open PRs

I notice there are a lot of older Renovate bot PRs still open, these open PRs block the creation of newer PRs. Maybe it's better to close the old PR if you're not going to merge it now, as that frees up a space for a newer PRs for another dependency. You can always get a fresh PR for the "closed PRs" from the Dependency Dashboard anyways.

Maybe bundle all React stuff into one PR?

Maybe you could put this in your packageRules array and bundle all React stuff into one PR?

    {
      "description": "Group react packages together",
      "matchPackagePatterns": ["react"],
      "groupName": "react packages"
    }

Wait until tests have passed before creating PR

Set the configuration option prCreation to "status-success". Branches with failing tests will remain in Git and continue to get updated if necessary, but no PR will be created until their tests pass.

You would then get a new heading in your Dependency Dashboard called: "Pending Status Checks". This would list all PRs that are waiting on passing tests.

https://docs.renovatebot.com/faq/#wait-until-tests-have-passed-before-creating-the-pr

ChristianMurphy commented 3 years ago

Thanks @HonkingGoose! Much appreciated!

   {
     "depTypeList": ["devDependencies"],
     "automerge": true
   },

Is this actually working for you? From what I can see, it appears that a maintainer is manually approving and merging them?

it appears to be (for example https://github.com/openequella/openEQUELLA/pull/2896#event-4594953161 https://github.com/openequella/openEQUELLA/commit/2a41b1a0224fb5df593a3e60ffe6a6f8c6b1df3b) openEquella is/was gearing up for a release, and folks were manually merging to speed the process. But the automerge appears to be working. :thinking:

I notice there are a lot of older Renovate bot PRs still open, these open PRs block the creation of newer PRs. Maybe it's better to close the old PR if you're not going to merge it now, as that frees up a space for a newer PRs for another dependency. You can always get a fresh PR for the "closed PRs" from the Dependency Dashboard anyways.

Good to know, thanks!

You would then get a new heading in your Dependency Dashboard called: "Pending Status Checks". This would list all PRs that are waiting on passing tests.

Interesting :thinking: The benefit of this approach, is that pending PRs don't count toward the PR total?

HonkingGoose commented 3 years ago

Pending Status checks PR count towards limit

The benefit of this approach, is that pending PRs don't count toward the PR total?

Well, I was hoping it would do that. But it turns out I was honking up the wrong tree here... Quote from the Renovate docs on the prConcurrentLimit property:

Limit to a maximum of x concurrent branches/PRs. 0 (default) means no limit.

Pending branches still count towards the prConcurrentLimit... 😄 🙈

Reduce noise with prCreation

I searched the docs for the prCreation configuration option, and you could use it to reduce the noise of new PRs.

prCreation

Renovate defaults to immediate but some like to change to not-pending. If you configure to immediate, it means you will usually get GitHub notifications that a new PR is available but if you view it immediately then it will still have "pending" tests so you can't take any action. With not-pending, it means that when you receive the PR notification, you can see if it passed or failed and take action immediately. Therefore you can customise this setting if you wish to be notified a little later in order to reduce "noise".

Documentation links:

Why are you using a PR limit

What's the reason you're using a PR limit anyways? The Renovate bot default is to allow a unlimited number of open PRs, but only open 2 per hour to avoid overwhelming CI resources.

I was thinking maybe some other workflow would be better? But I cannot give any advice if I don't understand why things are setup the way they are now. 😄

HonkingGoose commented 3 years ago

stabilityDays requirement for npm packages?

I don't know if you're aware but npm packages can be removed from the registry within 3 days after publishing a package. This could cause your build to fail later on because the package was removed. You can prevent this kind of thing by only allowing Renovate bot to open a PR for npm packages after 3 days.

Or you can keep things as they are, and get a automatic "rollback PR" from Renovate bot to return you to the next-highest version of the removed package.

So it's a choice of "do I want to prevent breakage" or do I want to fix it when it happens.

I'll let you decide what policy you like best. 😄

ChristianMurphy commented 3 years ago

What's the reason you're using a PR limit anyways?

That is a good question, it's been a while I don't :100: remember. IIRC it was a combination of two things:

  1. at the time renovate's default config did rate limit, 10 PR cap I believe, unlimited it for a bit, but then ran into 2.
  2. when renovate was first enabled, it was after a long dependency/code freeze there was some major dependency catch up needed which was overloading CI (https://github.com/openequella/openEQUELLA/pull/872 and https://github.com/openequella/openEQUELLA/pull/891), we capped it at 20 to give CI a break.

Not sure if it still makes sense to cap it at 20, thoughts @edalex-ian?

ChristianMurphy commented 3 years ago

stabilityDays requirement for npm packages?

Thanks! This isn't something we've run into, but good to have on the radar. stabilitydays sounds good to me, thoughts @edalex-ian?

ChristianMurphy commented 3 years ago

Maybe bundle all React stuff into one PR?

Some react things already seem to be bundled :thinking: https://github.com/openequella/openEQUELLA/pull/2460 Would

    {
      "description": "Group react packages together",
      "matchPackagePatterns": ["react"],
      "groupName": "react packages"
    }

combine more than the default config can/does?

HonkingGoose commented 3 years ago

Some more thoughts on bundling react packages

I was thinking that it might be a good idea to bundle the @types/react and @types/react-dom packages together, maybe you could bundle even more of those @types/react* packages together?

From looking at the package.json and Renovate dependency dashboard, it might be possible to group more of React into one PR, but I'm not sure if this is actually a good thing or not. You'll know better if those packages should be updated together, or can be safely updated separately.

Dependency Dashboard approval workflow

I'll throw this out there for your consideration:

The default behavior of Renovate is that you'll get a PR automatically whenever there's a update. You can override this and tell Renovate: "Only create a PR when I tick the box on the Dependency Dashboard". This is done by setting dependencyDashboardApproval to true.

You can configure Renovate to wait for approval for:

  • all package upgrades
  • major, minor, patch level upgrades
  • specific package upgrades
  • upgrades coming from specific package managers

The benefit is less PRs, as you're now in control when you get them. The drawback is that you are not made aware of new PRs right away, and (even worse) you don't know if any of the "pending approval" PRs pass or fail tests. So this is a bit of a mixed bag, but I wanted to let you know about the alternative workflow anyway.

https://docs.renovatebot.com/configuration-options/#dependencydashboardapproval

I think that's all I can contribute for now, I don't have any other obvious things that I see that I can fix/help with. 😄 I'll let you all think about the things that have been discussed. If you need more information/help from me, feel free to ping me. 😉

edalex-ian commented 3 years ago

What's the reason you're using a PR limit anyways?

That is a good question, it's been a while I don't 100 remember. IIRC it was a combination of two things:

1. at the time renovate's default config did rate limit, 10 PR cap I believe, unlimited it for a bit, but then ran into 2.

2. when renovate was first enabled, it was after a long dependency/code freeze there was some major dependency catch up needed which was overloading CI (#872 and #891), we capped it at 20 to give CI a break.

Not sure if it still makes sense to cap it at 20, thoughts @edalex-ian?

Correct. It was due to our CI platforms getting flooded - specifically Travis at the time. Our builds take around ~2hrs so we'd quickly find our CI would not have capacity to process our actual work. And indeed, we even experienced this again last week - even with the more generous quotas from GitHub Actions - where organising our release builds were delayed due to a sudden flood of renovate tickets. (Indeed we had to go and stop the builds so that we could get our release related builds complete.)

So it seems we still need it.

stabilityDays requirement for npm packages?

stabilitydays sounds good to me, thoughts @edalex-ian?

Wow! I didn't realise how easily you can remove an NPM package. This does seem like a good idea - and may also smooth and reduce the builds related to the culture of some projects releasing multiple patch releases in a single day. Let's take that for a spin.

Reduce noise with prCreation

I like the sounds of setting prCreation to not-pending. I guess this means it creates the branch in the background, awaits the result of the push CI build, and then creates the PR. If that's the case, we should set this up too.

HonkingGoose commented 3 years ago

If you want to setup stabilityDays for npm packages only, you can put a new object within the packageRules array like this:

  "packageRules": [
    {
      "description": "Wait 3 days before creating a npm update PR",
      "matchDatasources": ["npm"],
      "stabilityDays": 3
    }
  ],

Reduce noise with prCreation

I guess this means it creates the branch in the background, awaits the result of the push CI build, and then creates the PR. If that's the case, we should set this up too.

Quote from the Renovate docs on prCreation config option:

not-pending: Renovate will wait until status checks have completed (passed or failed) before raising the PR

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

edalex-ian commented 3 years ago

Let's keep this open for now, as we're also battling issues with package-lock and NPM 7. (e.g. #3275 and #3265)

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.