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

Preview changes to other files in onboarding branches #20797

Closed
jtdoepke opened this issue Mar 7, 2023 · 22 comments · Fixed by #20893
Closed

Preview changes to other files in onboarding branches #20797

jtdoepke opened this issue Mar 7, 2023 · 22 comments · Fixed by #20893
Labels
core:onboarding Related to the onboarding process priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality)

Comments

@jtdoepke
Copy link

jtdoepke commented Mar 7, 2023

What would you like Renovate to be able to do?

Renovate ignores changes to files other than the renovate.json config file in onboarding branches.

A common pattern for using the regex manager is to pass dependency details to Renovate in a comment e.g.

{
  "regexManagers": [
    {
      "fileMatch": [
        "(^|/|\\.)Dockerfile$",
        "(^|/)Dockerfile[^/]*$"
      ],
      "matchStrings": [
        "# renovate: datasource=(?<datasource>[a-z-]+?) depName=(?<depName>[^\\s]+?)(?: (lookupName|packageName)=(?<packageName>[^\\s]+?))?(?: versioning=(?<versioning>[^\\s]+?))?(?: registryUrl=(?<registryUrl>[^\\s]+?))?\\s(?:ENV|ARG) .+?_VERSION[ =]\"?(?<currentValue>.+?)\"?\\s"
      ]
    }
  ]
}

However, if you add those comments in the onboarding branch, the dependencies don't show up in the description of the pull request because Renovate generates that preview based on baseBranches.

I can work around this by temporarily adding

{
  "baseBranches": ["renovate/configure"]
}

to renovate.json, and then removing it before the merge, but it's inconvenient.

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

If the target branch of the onboarding pull request is in baseBranches, then Renovate should preview what the results of merging all files in the pull request would be, not just the renovate.json config file.

Is this a feature you are interested in implementing yourself?

No

@jtdoepke jtdoepke added priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality) labels Mar 7, 2023
@jtdoepke jtdoepke changed the title Previews changes to other files in onboarding branches Preview changes to other files in onboarding branches Mar 7, 2023
@rarkins rarkins added the auto:reproduction A minimal reproduction is necessary to proceed label Mar 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2023

Hi there,

Get your issue fixed faster by creating a minimal reproduction. This means a repository dedicated to reproducing this issue with the minimal dependencies and config possible.

Before we start working on your issue we need to know exactly what's causing the current behavior. A minimal reproduction helps us with this.

To get started, please read our guide on creating a minimal reproduction.

We may close the issue if you, or someone else, haven't created a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.

Good luck,

The Renovate team

@jtdoepke
Copy link
Author

jtdoepke commented Mar 8, 2023

Here is a minimal reproduction: https://github.com/jtdoepke/renovate-20797

@rarkins
Copy link
Collaborator

rarkins commented Mar 8, 2023

@RahulGautamSingh this highlights something about our onboarding which I had forgotten. While the config is read from the onboarding branch, Renovate still reads dependencies from the default branch.

I guess the current approach was considered a feature - Renovate can update the onboarding PR based on latest dependencies in the default branch without needing to push to the onboarding branch (which may be modified). It also means that baseBranches can work well.

I'm not going to close this, because it's a valid request to be able to do this, but I'm not sure we can without either loss of existing functionality or a major code rewrite.

@rarkins rarkins added core:onboarding Related to the onboarding process priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others reproduction:provided and removed priority-5-triage auto:reproduction A minimal reproduction is necessary to proceed labels Mar 8, 2023
@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 8, 2023

Then we should handle this before trying to reduce cloning for onboarding. Will look into it and let you know how much work is needed.

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 9, 2023

We still don't want to rebase/update the onboarding branch right? We onky want to update the PR body with the new extraction result.

@RahulGautamSingh
Copy link
Collaborator

Maybe we can add an option where the user gets to decide which branch he wants to picks as the baseBranch.
eg. useOnboardingBranchAsBase

If the option is true we handle modified onboarding branches differently else we use existing logic.

When an onboarding branch is modified

Existing logic New logic
No futher branch rebase No futher branch rebase
No extraction is performed Extraction will be performed
No PR body update PR body will be updated
We ensure a comment We ensure a comment

What do you say? @rarkins

@rarkins
Copy link
Collaborator

rarkins commented Mar 9, 2023

I'm not sure how to get this to work.

If the branch is unmodified (with our onboarding default config) we could either:

  • Use dependencies from default branch, but config from the onboarding branch, or
  • Refresh the onboarding branch each time the default branch is changed

Both should product the same result (in the PR body). The second one is less efficient because we commit more frequently.

The question from this feature request is what to do when the onboarding branch is modified.

I had one idea, not sure if it floats, but..

The onboarding branch will be merged into the default branch. Meaning whatever modifications which have been made will be merged in.

Maybe we can do this:

  • Clone the repo
  • git merge (locally) the onboarding branch into the default branch
  • proceed accordingly (kind of like we do today) by extracting agains the default branch (which now has the content from onboarding branch)

This will mean that default branch SHA we extract from won't be the same as what's on the server, but it shouldn't matter to the user (they are not exposed to SHAs during onboarding). We might need to think about it for caching though.

Second problem is that if the onboarding branch is conflicted with the default branch then we can't merge. But maybe we document that we can't proceed if the onboarding branch is in conflict - user needs to resolve it.

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 10, 2023

Can we use the workaround logic mentioned above by author?
We can introduce a new config option say useOnboardingAsBase. To enable this user can add it to the onboardingBranch thus modifying the branch.

The all we need to do is to add renovate/configure to the list of baseBranches when the onboardingBranch is modified and useOnboardingAsBase=true.

@rarkins
Copy link
Collaborator

rarkins commented Mar 10, 2023

And where would that new config option live? In the renovate.json in the onboarding branch?

It just doesn't seem elegant to me.

@RahulGautamSingh

This comment was marked as resolved.

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

@jtdoepke How were you able to get a onboardingPr generated for your reproduction as the chartfile.yaml is not recognised by Renovate? When I try the run finishes with
INFO: Repository has no package files - skipping

Did you makes changes to the extraction logic and add chartfile.yaml to fileMatch array?

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

@rarkins I tried what you mentioned and it will work.

We will need the following additions:

  1. call mergeBranch when onboaring branch is modified.

  2. mergeBranch function pushes to the remote base branch (expected as we use it for automerge) but for this case we can't push the changes. So we weill need to
    either create a duplicate function that does the same except for pushing
    OR add an extra parameter to exising fn that decides whether to push to remote or not.

  3. It would be better if we add some extra code to revert the local baseBranch to its pre-merge state after we have performed extraction so that checks like isBranchConflcited which are performed during ensureOnboarindPr stage don't throw error like:

 "message": "fatal: There is no merge to abort (MERGE_HEAD missing).\n",

@rarkins
Copy link
Collaborator

rarkins commented Mar 11, 2023

Why not merge every time if it works in both cases?

And yes, let's make sure it's local merge only.

Why/when are we calling isBranchConflicted?

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

We call isBranchConflicted in ensureOnboardingPr function.
image

@rarkins
Copy link
Collaborator

rarkins commented Mar 11, 2023

I expected that type of logic would be executed before we merge

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

We don't perform merge for onboardingPrs so that check shouldn't be necessary while processing the branch.

@rarkins
Copy link
Collaborator

rarkins commented Mar 11, 2023

I thought we were just discussing merging it?

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

My bad, I thoguht you meant why it wasn't there to begin with.
Now that we want to merge beforehand, it will be efficient to check it before merging aand store the data to be used for later calls.

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

Also, there is no point in merging if the onboardingBranch isn't modified as the changes will be same either way. But if you think it makes the flow/dessign better then I will try to see where it can be placed in the logic-flow.

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

There is one more issue:
The branchIsConflicted result isn't stored because we don't create cache for onbaording branches.

The possible solutions can be to:

  1. add onboardingbranch to branch cache Or
  2. use the global object to store the value during the run. (we do this for modified result already)

@rarkins
Copy link
Collaborator

rarkins commented Mar 11, 2023

We definitely should add onboarding branch info to the cache

@RahulGautamSingh
Copy link
Collaborator

RahulGautamSingh commented Mar 11, 2023

I can begin work now:

  1. Add onboarding branch to branch-cache (need to submit a new PR)
  2. Update feat(onboarding): use cache to check if repo is onboarded #20733 PR and get it merged.
  3. Work on this one feat(onboarding): lazy onboarding #20702
  4. Start work on a draftPr for this issue (I have already tested a basic impementation here)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core:onboarding Related to the onboarding process priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants