dependabot / dependabot-core

🤖 Dependabot's core logic for creating update PRs.
https://docs.github.com/en/code-security/dependabot
MIT License
4.63k stars 990 forks source link

rebase command runs an action but doesn't actually rebase the branch #10118

Closed smlx closed 1 month ago

smlx commented 2 months ago

Is there an existing issue for this?

Package ecosystem

gomod, docker

Package manager version

n/a

Language version

n/a

Manifest location and content before the Dependabot update

https://github.com/smlx/goodwe-exporters/blob/a559b4c02943badf09483304f2753d905ed9de04/Dockerfile#L1

dependabot.yml content

https://github.com/smlx/goodwe-exporters/blob/a559b4c02943badf09483304f2753d905ed9de04/.github/dependabot.yaml#L1

Updated dependency

alpine from 3.19 to 3.20

What you expected to see, versus what you actually saw

On this PR I commented @dependabot rebase, and that kicked off an action running dependabot which completed successfully. But the PR was never actually rebased, so it still can't be merged.

Native package manager behavior

n/a

Images of the diff or a link to the PR, issue, or logs

https://github.com/smlx/goodwe-exporters/pull/69

Smallest manifest that reproduces the issue

No response

mbiggs-gresham commented 2 months ago

Same with yarn ecosystem. recreate doesn't work either.

elchininet commented 2 months ago

I have seen this issue but it is not the same in my case. In my repos this started just one week ago, rebase command just adds a thumbs up, add a comment at the beginning of the MR and later it removes the comment without rebasing anything.

Example: https://github.com/elchininet/isometric-css/pull/143

sambauers commented 2 months ago

I have seen behaviour that might be the same or possibly related. Attempts to rebase sometimes come up with unresolvable conflicts. I was able to replicate this behaviour outside of Dependabot PRs manually. The cause seems to be the way in which PNPM lock files identify packages. For example this is a fairly normal identifier of a package inside a PNPM lock file:

/@mui/material-nextjs@5.15.11(@emotion/cache@11.11.0)(@mui/material@5.15.21)(@types/react@18.3.3)(next@14.2.4)(react@18.3.1)

With the above example, if you happen to have seperate PRs to update @mui/material@5.15.21 and next@14.2.4 at the same time, merging one will cause the other PR to have unreconcilable conflicts which require manual resolution.

Intuitively, I would expect that running @dependabot rebase would fix this, but it doesn't, and @dependabot recreate doesn't seem to do anything either.

jpaakko commented 2 months ago

We have exactly the same issue with multiple different PRs in multiple repositories. The issue started a bit over a week ago for Dependabot PRs created on June 22nd and 23rd. As a quick fix, we resolved these PRs by rebasing them manually but that's not something that we want to keep doing for future PRs. The issue occurred again for us for PRs created on June 29th and 30th.

When running @dependabot rebase or @dependabot recreate, Dependabot adds a thumbs up emoji to the comment and the "⚠️ Dependabot is rebasing this PR ⚠️" text appears in the PR description. However, once the action has run and the text is removed from the PR description, nothing happens and Dependabot doesn't rebase the PR.

The Dependabot logs don't show any errors and the logs look the same as for PRs where rebasing works. We've tested running Dependabot using the built-in Dependabot application and on GitHub actions but the issue occurs irrespective of how Dependabot is run.

One thing that is common to all our PRs where rebasing doesn't work is that they are part of a Dependabot group. However, the issue doesn't seem to concern all Dependabot groups as Dependabot has been able to rebase some PRs that concern grouped updates.

Package ecosystem: Yarn (npm)

mbiggs-gresham commented 2 months ago

Can confirm what @jpaakko said, I hadn't noticed before but it seems to be for group PR's. Just tried a rebase on both an individual dependency and a group one. The individual one worked and successfully rebased, the group one gave a thumbs up, said it was rebasing but ultimately did nothing.

Mr0grog commented 2 months ago

I am also seeing this frequently in a project I recently started contributing to with Ruby/Bundler dependencies.

I hadn't been paying attention before, but the last two times I saw this definitely was for grouped dependencies, so that could definitely be related.


Update: this absolutely seems related to grouped dependencies. Had another repo with some grouped and non-grouped open PRs and got the same results as @mbiggs-gresham.

jpaakko commented 2 months ago

I previously assumed that the issue didn't concern all grouped update PRs as some of our grouped update PRs had been successfully merged. Thus, I had mentioned:

the issue doesn't seem to concern all Dependabot groups as Dependabot has been able to rebase some PRs that concern grouped updates

However, today I went through all our grouped update PRs for the last couple of weeks. I noticed that the grouped update PRs that had been successfully merged didn't have any @dependabot rebase or @dependabot recreate commands since the PRs had already been up-to-date with the main branch. That explains why Dependabot had been able to merge these without any issues.

In contrast, all of our grouped update PRs during the last couple of weeks that have @dependabot rebase or @dependabot recreate comments have been stuck.

Based on this finding, it looks like rebasing grouped update PRs is currently broken for all grouped update PRs. The last time that Dependabot has been able to successfully rebase a grouped update PR in our repositories is on June 21st.

broksonic21 commented 2 months ago

I had filed https://github.com/dependabot/dependabot-core/issues/10135 as well, which is same scenario:

Feel like it's been for a week or two that this has been going on. Seeing it on both bundler and npm scenarios.

NatoBoram commented 2 months ago

We're relying on Dependabot at @CodeRabbitAI and we're affected by this Dependabot outage

boomshakalakah commented 2 months ago

I don’t know ruby so I can’t find any suspicious code, but is there a chance that #10002 broke it? The timing lines up, and it changes behaviour around grouped dependencies.

Regardless, here’s potentially relevant stuff from my update logs.

I triggered @dependabot recreate on the PR for a group called tests, which currently has merge conflicts against main.

In the logs for a different PR, I can see

updater | 2024/07/03 18:31:10 INFO <job_851126648> Detected existing pull request for 'tests'.
 updater | 2024/07/03 18:31:10 INFO <job_851126648> Deferring creation of a new pull request. The existing pull request will update in a separate job.

In the update logs for the tests group…

I’m seeing "dependency-has-directory":true, which is referenced in that PR.

Also,

updater | 2024/07/03 18:38:58 INFO <job_851129715> Starting job processing
 updater | 2024/07/03 18:39:00 INFO <job_851129715> Starting PR update job for <REDACTED>
 updater | 2024/07/03 18:39:00 INFO <job_851129715> Updating the 'tests' group
 updater | 2024/07/03 18:39:00 INFO <job_851129715> Updating the / directory.
updater | 2024/07/03 18:40:03 INFO <job_851129715> Target versions have changed, existing Pull Request should be superseded
 updater | 2024/07/03 18:40:03 INFO <job_851129715> Creating a new pull request for 'tests'
 updater | 2024/07/03 18:40:12 INFO Results:
 updater | +-----------------------------------------------------------------------------------------------+
 updater | |                              Changes to Dependabot Pull Requests                              |
 updater | +---------+-------------------------------------------------------------------------------------+
 updater | | created | @testing-library/dom ( from 10.1.0 to 10.3.0 ), cypress ( from 13.12.0 to 13.13.0 ) |
 updater | +---------+-------------------------------------------------------------------------------------+

However, those target versions haven’t actually changed. They’re the same as the versions in the existing PR.

image-4.png

In the end, it hasn’t created a new PR or recreated commits on the existing PR.

From what I can tell, it looks like it’s ending up in this else block when it should be matching this elsif above it.

EDIT: This setup is configured for an npm/yarn project.

dotLou commented 2 months ago

I think @boomshakalakah is on the right track here. I'm not super familiar with this code base, but I did notice that we've got spots where we create a dependency set that doesn't include the newly added directory key:

https://github.com/dependabot/dependabot-core/blob/e8d8a1268ea61304e939ba9ab963e249cac5b241/updater/lib/dependabot/updater/operations/refresh_version_update_pull_request.rb#L204-L210

This would mean that the check in https://github.com/dependabot/dependabot-core/blob/17fe8bfa40b98bb769691b882f0be3f514c852a1/updater/lib/dependabot/updater/group_update_refreshing.rb#L57 would fail because the sets don't match anymore

Edit:

It's worth noting that because there's no single method that creates these sets, there are also other areas that create them without this new value such as https://github.com/dependabot/dependabot-core/blob/e8d8a1268ea61304e939ba9ab963e249cac5b241/updater/lib/dependabot/updater/operations/refresh_security_update_pull_request.rb#L199-L205

boomshakalakah commented 2 months ago

Fun update: I now have two dependabot PRs open for the same group.

Now that there's a version change for some packages in the group, it automatically created a second PR. It did not update or close the original PR.

image-5.png

image-6.png

slifty commented 2 months ago

In case more examples provide any clues, I've also been facing this:

  1. https://github.com/PhilanthropyDataCommons/service/pull/1095
  2. https://github.com/PhilanthropyDataCommons/service/pull/1092
  3. https://github.com/PhilanthropyDataCommons/service/pull/1080

All three cases were for a grouped dependency (I'll check next time there is a non-grouped dependency to see if that case works)

slifty commented 2 months ago

I don't know if others had this happen as well but 2 minutes ago dependabot did finally issue the force push on https://github.com/PhilanthropyDataCommons/service/pull/1095, merged, and now everything is as it should be.

It seems this bug may be fixed?

boomshakalakah commented 2 months ago

I don't know if others had this happen as well but 2 minutes ago dependabot did finally issue the force push on https://github.com/PhilanthropyDataCommons/service/pull/1095, merged, and now everything is as it should be.

It seems this bug may be fixed?

LGTM

martinpitt commented 2 months ago

Confirmed in https://github.com/cockpit-project/cockpit-ostree/pull/651#issuecomment-2226164722 , works again. Thanks!

elchininet commented 2 months ago

Confirmed also on my pending pull requests. It is working now.👍🏼

jakecoffman commented 1 month ago

The rebase regressions are fixed now. The main cause for it was directory hasn't yet been sent to the server, and so it's always blank, as @dotLou pointed out.

This fixes it going forward because it handles directory being present or not: https://github.com/dependabot/dependabot-core/pull/10224

The reason you saw it started working again before this fix was I flipped the feature flag back off to mitigate the issue.

Sorry about that and thanks for your patience! Thanks for all the reports!