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

Use cache to prevent cloning when onboarding repos #17839

Closed
RahulGautamSingh opened this issue Sep 17, 2022 · 8 comments
Closed

Use cache to prevent cloning when onboarding repos #17839

RahulGautamSingh opened this issue Sep 17, 2022 · 8 comments
Assignees
Labels
core:cache core:onboarding Related to the onboarding process priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:blocked Issue is blocked by another issue or external requirement type:refactor Refactoring or improving of existing code

Comments

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Sep 17, 2022

Describe the proposed change(s).

Today, while processing repos that are not onboarded, we clone each repo. We can prevent cloning of the repos for which the following conditions match:

  1. Onboarding branch state same as last run
  2. Base branch state same as last run

I've included the situations that cause in cloning on re-runs below, along with potential workarounds which use cached info:

1. The computation of isOnboarded

Working:

  • Check for a config file in base branch
    a) first using API , then the file is absent
    b) clone repo and check for the file again

Problem:
API call always returns false for unmerged onboarding PR so we endup cloning on each re-run.

Solution: 
Cache an onboarding object while creating onboarding PR and delete it when the PR is merged. That way it's presence will be enough to decide isOnboarded=false.
We will also need to add logic to invalidate the cache whenever the baseBranch is updated.

2. Rebasing the onboarding branch on each re-run

This one is a tricky problem to solve cause it has an edge case:

(a) When is rebasing required

  • base branch SHA differs
  • user adds a preset to their org (maybe its so rare that we dont need to handle this)

Tricky because onboardingConfig needs updating only when a user/org adds a preset after the onboarding PR was created by us.

3. checkoutBranch(onboardingBranchName)

  • This happens on all the runs
  • solution: conditionally checking out a branch.

4. mergeRenovateConfig()

Working:

  • In this function we call another function detectConfigFile() which
    a) first uses the api calls to detect any config files in repo then if not found
    b) clones the repo and tried to look for it locally

Solution:

  • Check for onboardingCache if present use the config stored in it. Else follow normal routine.
    Not sure if this will need invalidation or maybe the invalidation in isOnboarded be enough to handle it here as well.
@RahulGautamSingh RahulGautamSingh added type:refactor Refactoring or improving of existing code status:requirements Full requirements are not yet known, so implementation should not be started priority-5-triage labels Sep 17, 2022
@RahulGautamSingh

This comment was marked as outdated.

@RahulGautamSingh

This comment was marked as outdated.

@RahulGautamSingh

This comment was marked as outdated.

@RahulGautamSingh RahulGautamSingh self-assigned this Sep 17, 2022
@rarkins rarkins added status:ready priority-2-high Bugs impacting wide number of users or very important features core:cache and removed status:requirements Full requirements are not yet known, so implementation should not be started priority-5-triage labels Nov 25, 2022
@HonkingGoose HonkingGoose added the core:onboarding Related to the onboarding process label Jan 18, 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 Feb 21, 2023
@rarkins
Copy link
Collaborator

rarkins commented Feb 24, 2023

Blocked by #20537

@rarkins rarkins added status:blocked Issue is blocked by another issue or external requirement and removed status:ready labels Feb 24, 2023
@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Mar 2, 2023

After the PR #20537 is merged we won't need to handle the issues (2) and (3) mentioned in the description.
Thus only the (1) & (4) issue needs to be handled.

A possible solution is discussed in the description I am repeating it here again.

For issue (1) (prevent cloning just to check onboarding state)

  • Cache commitSHA of default branch in OnboardingCache
    Then on next run:
  • If commitSHA is same - skip isOnboarded check as the repo isn't onboarded.
  • Else fallback to old isOnboarded logic.

@RahulGautamSingh RahulGautamSingh self-assigned this Mar 2, 2023
@rarkins
Copy link
Collaborator

rarkins commented Mar 2, 2023

If the commit sha of the default branch and the commit sha of the onboarding branch are unchanged, we 100% should not clone or do any work.

If the commit sha of the onboarding branch is changed then we should clone and do work.

If only the commit sha of the default branch is changed then I would like to skip cloning unless the onboarding PR has been checked.

Like this:

  • Is the onboarding PR open?
    • If yes, and either branch sha changed or checkbox is ticked, then do work
    • Else do no work
  • Else do work (keeping in mind edge cases like onboarding=disabled, requireConfig, etc)

@RahulGautamSingh
Copy link
Collaborator Author

RahulGautamSingh commented Mar 2, 2023

Here we decided to skip the PR/branch if checkbox is unchecked regardless the onboarding branch commit sha has changed or not.

Except this part what you said above seems fine.

@rarkins
Copy link
Collaborator

rarkins commented Mar 2, 2023

Yes, please use this logic instead (it makes sense to run/update if the user modifies the config in the onboarding branch)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core:cache core:onboarding Related to the onboarding process priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:blocked Issue is blocked by another issue or external requirement type:refactor Refactoring or improving of existing code
Projects
None yet
Development

No branches or pull requests

3 participants