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

Lazy onboarding PR refresh #20537

Open
rarkins opened this issue Feb 21, 2023 · 16 comments
Open

Lazy onboarding PR refresh #20537

rarkins opened this issue Feb 21, 2023 · 16 comments
Labels
core:onboarding Related to the onboarding process performance Performance-related improvements priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)

Comments

@rarkins
Copy link
Collaborator

rarkins commented Feb 21, 2023

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:

  • a commit is made to the onboarding branch, or
  • users tick a (new) checkbox in the onboarding PR which says something like "Refresh this PR".

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

@rarkins rarkins added type:feature Feature (new functionality) priority-2-high Bugs impacting wide number of users or very important features status:requirements Full requirements are not yet known, so implementation should not be started performance Performance-related improvements labels Feb 21, 2023
@HonkingGoose
Copy link
Collaborator

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
Copy link
Collaborator

RahulGautamSingh commented Feb 22, 2023

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
Copy link
Collaborator Author

rarkins commented Feb 22, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator Author

rarkins commented Feb 22, 2023

It's not what I expected

@RahulGautamSingh
Copy link
Collaborator

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
Copy link
Collaborator Author

rarkins commented Feb 23, 2023

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
Copy link
Collaborator

RahulGautamSingh commented Feb 25, 2023

Summarizing:

  • From now on we don't update onboardingPrs
  • Add a checkbox which will give user option to update the onboardingPr
  • If checkbox:
  1. unchecked -> end run
  2. checked -> IF config has changed "rebase/retry" the pr ELSE "refresh" the pr

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

@rarkins
Copy link
Collaborator Author

rarkins commented Feb 25, 2023

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:

  • Keep it behind a config option (maybe the existing one, or rename it)
  • If PR is newly created or unmodified, add checkbox with text Rebase/refresh this PR from default branch
  • If PR is modified, checkbox should have text Rebase/refresh this PR from default branch (Warning: custom config changes will be lost)
  • When processing an existing onboarding PR in subsequent runs, skip the repo if this checkbox is present and empty (i.e. unchecked)

So the idea is:

  • Admins can enable this, if their platform supports
  • If enabled, onboarding repos will cause very little work each run (no cloning, no lookups), unless the user has requested the work

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 1, 2023

  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
Copy link
Collaborator Author

rarkins commented Mar 1, 2023

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
Copy link
Collaborator

RahulGautamSingh commented Mar 1, 2023

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
Copy link
Collaborator

RahulGautamSingh commented Mar 1, 2023

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
Copy link
Collaborator

RahulGautamSingh commented Mar 1, 2023

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
Copy link
Collaborator Author

rarkins commented Mar 1, 2023

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

@RahulGautamSingh
Copy link
Collaborator

Note: This PR #20702 isn't enough to prevent cloning. More work need to be done.

@HonkingGoose HonkingGoose added the core:onboarding Related to the onboarding process label Mar 22, 2023
@rarkins rarkins added status:in-progress Someone is working on implementation and removed status:requirements Full requirements are not yet known, so implementation should not be started labels May 27, 2023
@rarkins rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others and removed priority-2-high Bugs impacting wide number of users or very important features labels Jun 27, 2023
@rarkins rarkins removed the status:in-progress Someone is working on implementation label Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core:onboarding Related to the onboarding process performance Performance-related improvements priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants