depfu / feedback

🤔 Question, bugs and feedback for Depfu
https://depfu.com
MIT License
8 stars 4 forks source link

Auto-merging conflicts doesn't work if target dep gets updated in base branch #28

Closed anym0us closed 5 years ago

anym0us commented 5 years ago

Let's imagine the situation when

  1. Depfu [bot] creates a PR to update some dependency.

  2. Until the PR gets closed, the same dependency gets updated in base/master branch. (I saw case when it was caused by Depfu as well: when another Depfu's PR gets merged).

If versions doesn't match, the conflict occurs. However @depfu rebase command doesn't help to resolve it.

Desired change:

Here's the real example:

  1. Depfu has suggested the following update (and created a PR for this): Upgrade eslint-plugin-react: 7.11.1 → 7.12.4 (minor)

  2. In the meanwhile eslint-plugin-react has been updated in master to 7.12.0 (I saw the cases when similar updates were caused by Depfu as well)

As a result, package.json and yarn.lock are now conflicting files in the PR. @depfu rebase doesn't work to resolve it, however the changes are quite obvious.


Few screenshots:

theflow commented 5 years ago

Hi @anym0us 👋

Thanks for the detailed bug report!

Your desired change is actually exactly how @depfu rebase works today.

Unfortunately in your case for this specific update there seems to be a problem with your yarn.lock that causes the rebase to fail. I'll follow up with you via email since it relates to a private repo and is very specific.

I'll also look into exposing a failing rebase better, so you don't think nothing is happening.

Cheers, Florian

MilosMosovsky commented 5 years ago

@theflow Thanks for the info, however the problem with yarn.lock is caused by depfu (especially when major version upgrade occurs in 1 PR and in other it gets conflict after the 1st PR is merged).

Is there any possibility to rerun depfu PR creation by comments ? e.g something like @depfu recreate which will close the PR and creates new based on fresh master ? Or even doesn't close the current PR but just does git reset --hard origin/{TARGET_BRANCH} and run the command for package upgrade again ?

The case is not private-repo specific, it's case when you integrate depfu into long living project which has many pending updates. It's almost 9/10 times when you update major versions of 2 packages they will create conflicting yarn.lock that's the nature of yarn.

Having command which will re-run the creation of PR is in my opinion

1) only correct way how to to do upgrade of npm packages with yarn command (I did many updates in many projects and auto-rebase, auto-merge causes problems for yarn.lock even if git states the merge was successful. yarn.lock never should be merged/rebased with auto feature neither manually it should always be result of running either yarn install or yarn upgrade thus auto-generated by yarn

2) It will solve the current problem

theflow commented 5 years ago

Hi Miloš,

I understand that the Depfu PRs tend to generate conflicts, especially in the beginning of using it. But that's exactly why we built the @depfu rebase and auto-rebasing. What it does is:

So if it didn't do that for a specific pull request of yours or even all of them, please let me know and I'll look into the specific case. I've checked the update from the screenshot and there was an exception that was causing the depfu rebase to not run at all, which is why I explained it was specific to an edge case in that repo.

Cheers, Florian

MilosMosovsky commented 5 years ago

@theflow Thanks for the explanation, then it makes sense, I thought that @depfu rebase is doing git rebase on master, not checking out package.json and yarn.lock and re-running command. Thanks for the info!.You can proceed than in email with @anym0us he should have all the info.

Thanks for your quick support! Let us know if any further explanation/reproduction is needed.

theflow commented 5 years ago

Thanks @MilosMosovsky, I've reached out to him already. We were discussing different names for the command, but even though it's not 100% correct, it's what most users were looking for and expecting. Naming is hard :)

MilosMosovsky commented 5 years ago

@theflow I came with more investigation what actually happened in our case. We had installed storybook@5.0.5 (locked to yarn.lock)

Depfu created 2 PR's

We merged Update @storybook/theming PR as first and Update all of storybook got conflicts as it included @storybook/theming as well.

Commits · toptal:picasso 2019-04-10 14-31-10

theflow commented 5 years ago

Yeah, in that case we missed that @storybook/theming is part of the storybook packages. I've fixed that now for future updates. But you'll still get conflicts, even for unrelated updates, can't really avoid that.