renovate-approve not working with forking-renovate

[rsimha]

Background:

The AMP Project has been using forking-renovate for a couple years now, and I'm in the midst of trying to up our game by enabling auto-review and auto-merge for upgrade PRs that pass other CI checks. However, I haven't been able to get things to work after multiple tries, and figured I'd check in here for help. Please feel free to move this issue if this is the wrong repo.

Details:

• Our repo is located at https://github.com/ampproject/amphtml
• Contributions are normally made via forks, so we installed https://github.com/apps/forking-renovate
• All upgrade PRs are at https://github.com/ampproject/amphtml/pulls/renovate-bot
• I installed https://github.com/apps/renovate-approve so PRs that pass CI are auto-merged
• Our renovate config is located at https://github.com/ampproject/amphtml/blob/main/.renovaterc.json
• I've set automerge and assignAutomerge as true for the config groups that we want auto-merged

Problems:

• renovate-approve has not been added as a reviewer / approver for any PR thus far
• If I manually review / approve a PR such that all PR statuses are green, the PR is still not being auto-merged

Questions:

• Is auto-merge supposed to work with forking-renovate?
• Are the steps listed above the correct steps? (Or did I misconfigure something?)
• Any other steps I should've taken?

Thanks in advance for the help!

[rarkins]

Hi Raghu! Nice to chat with you again.

The Renovate Approve app/bot is currently configured to only approve automerging PRs from the main renovate[bot] account and not forking-renovate[bot]. The reason is that we assumed anyone using Forking Renovate does so because they don't want to grant write access, and therefore the forking bot would never be automerging in the first place.

So therefore the behavior you're seeing is expected, but we can possibly change it.

For the bot to automerge, you'd need to add it as an allowed committer against your main branch. If that's OK with you to do then in theory automerge should be technically possible and we can look into what code changes are necessary in the core Renovate logic as well as the Renovate Approve bot too.

[rsimha]

Nice to chat with you too, Rhys! And thanks for the quick and detailed reply.

Yes, we certainly can give the bot account write permissions to the main branch. This is in keeping with AMP's governance model, where external contributors are allowed to merge their PRs once we're satisfied that all CI checks have passed.

It's very kind of you to suggest the possibility of enhancing the bot's existing functionality to support this. Let us know if there's anything we can do at our end to help with this.

Thank you!

[rarkins]

I'll look into this and then update you here about progress. The changes may involve both this repo as well as the main Renovate one. FYI in case it wasn't clear, it would be @renovate-bot which would need write access and not the Forking Renovate bot. We use @renovate-bot to create the commits and PRs so that Google's CLA bot is happy and doesn't complain about multiple users :)

[rsimha]

Perfect. Thanks once again!

[rsimha]

I see that the auto-approvals from renovate-approve have started coming in. Thanks for the really quick work!

Now, I'm working to make sure our CI checks are satisfied by those approvals and go all-green. (We use a custom code-ownership solution that applies a blocking CI check until it is satisfied that the approver(s) of a PR are also the rightful owners of the files that were changed.)

Once this is done, the coast will be clear to see if forking-renovate can indeed do the auto-merging.

[rsimha]

Two updates:

• I've configured our code ownership check to recognize @renovate-bot as an owner of amphtml package updates.
• I've granted @renovate-bot permissions to review and merge PRs to amphtml by inviting it to the Reviewers (amphtml) team of the ampproject org. (The invitation shows as pending, so I wonder if it needs to be accepted.)

[rarkins]

I think the next step is that we need to get one automerging PR that's "green". Right now they all have at least one failing check:

Therefore Renovate won't even attempt to merge, so we can't progress.

[rsimha]

• I've granted @renovate-bot permissions to review and merge PRs to amphtml by inviting it to the Reviewers (amphtml) team of the ampproject org. (The invitation shows as pending, so I wonder if it needs to be accepted.)

It looks like this step didn't have the desired effect of recognizing @renovate-bot as a valid reviewer. (I'm guessing this is because the account cannot accept an invitation to join a github team.) I'll attempt to find a workaround for this in our code-ownership system, and post an update here when we've figured it out. Meanwhile, thanks for the continued support!

[rarkins]

Actually that is a "real" user account (not an app) so I can log in as it and check for any invitations.

[rsimha]

Actually that is a "real" user account (not an app) so I can log in as it and check for any invitations.

👍 I'm fairly certain automerge will work if you do this. (I will still attempt to find a workaround via our code-ownership system because in future, we would like a way to not have to rely on this invitation mechanism for other bot reviewers.)

[rarkins]

When logged in as @renovate-bot, I cannot find any invitation including at https://github.com/ampproject/amphtml/invitations

I will close this issue now though because we got the approve part working. To continue the discussion, can you raise a Discussion here? https://github.com/renovatebot/renovate/discussions

[rarkins]

Actually.. I just found it under the ampproject org instead, so it's now all set.

[rsimha]

Thanks for following up, @rarkins! The good news is that PRs with automerge enabled are now passing all checks. For example, ampproject/amphtml#34203. I'll keep an eye on it for a while and post on the discussions group if it doesn't get automerged.

[rarkins]

Did you lose patience? :)

[rsimha]

Hah! No, I realized that we hadn't yet enabled auto-merge for the amphtml repo when this PR was created, so I did that and figured I'd clear out the backlog and look at tomorrow's PRs. Almost there 😃

[rarkins]

I haven't done a code inspection yet to check if it will attempt to automerge or not, but let's get a candidate PR ready and if it doesn't then we can check the logs and get an idea if it gives up too soon because it was written to assume that forked PRs can't be automerged.

[rsimha]

Here's a candidate PR: ampproject/amphtml#34219

• Regular CI checks are all green.
• Code ownership check is green, which means yesterday's permission changes have taken effect.
• GitHub UI shows that the automerge feature is enabled for the repo.
• PR description says this update is supposed to have automerge enabled.
• However, automerge hasn't been enabled yet. (I'd imagine if everything was in place, this would've happened during PR creation?)

[rsimha]

Checking in to say that after a week in action, automatic approvals have been working very nicely for us. Thanks once again for enhancing renovate-approve!