renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
16.7k stars 2.17k forks source link

Lazy onboarding PR refresh #20537

Open rarkins opened 1 year ago

rarkins commented 1 year ago

What would you like Renovate to be able to do?

Avoid refreshing onboarding PRs every time there's a commit to the default branch. Onboarding PRs sometime remain open for a long time and users don't need it refreshed non-stop.

If you have any ideas on how this should be implemented, please tell us here.

For platforms which support checkboxes, only refresh onboarding PRs when either:

If the PR isn't checked or the branch not modified, skip cloning and any other work.

We might need to do some brainstorming on whether the first point requires repositoryCache to be enabled or not. One alternative is to embed the branch commit SHA into the PR body

Is this a feature you are interested in implementing yourself?

No

HonkingGoose commented 1 year ago

We should explain the refreshing behavior in the PR body text for the Onboarding PR:

On platforms that support checkboxes, Renovate only refreshes this PR when you or somebody else:

  • pushes a new commit to the onboarding branch
  • selects the "Refresh this PR" checkbox
RahulGautamSingh commented 1 year ago

I don't undertand point (1) If a new commit is made to the onboading branch then its modified and we would want to ignore it.

rarkins commented 1 year ago

I thought we are clever enough to check if the only change to the branch is renovate.json files, but maybe I imagined that or it was old capabilities I removed for some reason. Can you double check?

RahulGautamSingh commented 1 year ago

Checked We prevent rebase if a new commit is added to the onboarding branch.

That aside I tried to see how the the recently added onboardingRebaseCheckbox feature worked. I found it odd that the branch isn't rebased after the checkbox is checked when the branch has been modified previously. Is that expected behaviour?

rarkins commented 1 year ago

It's not what I expected

RahulGautamSingh commented 1 year ago

Repeating what you said, Ignore the PR if a checkbox isn't checked this alone should be fine I think for onboarding PRs.

For platforms that don't support a checkbox maybe we can modify the prTitle or ask user to add a comment.

Implementation wise: We can use onboardingRebaseCheckbox:true in our hosted config and I will fix the feature to work as expected. ie. rebase regardless of any modifications when the box is checked.

rarkins commented 1 year ago

I had half forgotten about that feature. Actually, there's two different possibilities which checkboxes could satisfy:

  1. Refresh the branch (if config unchanged)
  2. Rebase/restart the branch (if config changed)

They're similar concepts but in the latter you'd wipe away your renovate.json changes

RahulGautamSingh commented 1 year ago

Summarizing:

I don't get what "refresh" means in this context. I assume its just refers to the normal run.

rarkins commented 1 year ago

I'm thinking of changed requirements:

First of all, we need to be prepared that not all platforms support checkboxes, so we need to revert back to normal behavior if no checkbox is present in the PR.

Then, it seems like we should have at most one checkbox in an onboarding PR, which will "refresh" the onboarding PR from main either way, but in the case that the config is modified, it should display a warning next to the checkbox that custom changes will be lost.

So logic is like this:

So the idea is:

RahulGautamSingh commented 1 year ago
  1. Currently onboardingRebaseCheckbox only handles rebase/refresh of the PR body. I think we can use it for both PR body as well as branch rebase. It is also well written ie. handles the case where the platform doesn't support checkboxes. You can confirm this and I will submit a draft PR today.

  2. Aside this, can we try the prTitle change for rebase/refresh in platforms that do not support checkboxes? The logic is already inplace for normal PRs, just need to use it for onboarding ones too.

rarkins commented 1 year ago

Do I understand correctly that you are proposing we can reuse the existing feature almost as-is, and it satisfies my use case?

BTW I'm most interested in GitHub for now

RahulGautamSingh commented 1 year ago

Yes with minor additions. When checkbox was ticked Previous: Only update the PR body and same flow from branch. New: Try to update both branch and PR.

RahulGautamSingh commented 1 year ago

I think we should not use 2 messages for the rebase checkbox because then we will have to call isBranchModified on each run, since we don't cache info for onboardingBranch.

Instead we can add a diffrent msg that warns user that opting for rebase on a modified PR will cause the changes to be lost.

RahulGautamSingh commented 1 year ago

I think we should not use 2 messages for the rebase checkbox because then we will have to call isBranchModified on each run, since we don't cache info for onboardingBranch.

If we use the idea you suggested to embed branch commitSHA into PR body.

The flow to check for modified branch will be like below I assume:

  1. Get the PR body (has embeded branch commit SHA)
  2. Get the lastest commit of onboardingBranch using api
  3. Compare
rarkins commented 1 year ago

I'm ok with us warning about changes being lost as a first step

RahulGautamSingh commented 1 year ago

Note: This PR https://github.com/renovatebot/renovate/pull/20702 isn't enough to prevent cloning. More work need to be done.