dependabot / dependabot-core

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

`@dependabot recreate` does not actually recreate the commit #4652

Open jeffwidman opened 2 years ago

jeffwidman commented 2 years ago

I had a dependabot PR that I was munging around with trying to see what additional changes were required to get CI to pass.

However, I accidentally fat-fingered some git commands and dropped the original commit from Dependabot, both locally and on the remote.

So I did a @dependabot recreate since the description of that command says:

@dependabot recreate will recreate this PR, overwriting any edits that have been made to it

However, when Dependabot recreated the PR, it didn't reset the commit title or description. It merely reset the code changes of the commit.

I think this is a bug because recreating the PR should recreate the commit metadata, title, description, etc. Honestly, that's a big part of the usefulness of Dependabot, is the detailed commit messages listing what changed and links to more info. Very useful for later git blame spelunking when trying to figure out what may have caused a problem.

Notes:

mctofu commented 2 years ago

I think this happens because a recreate triggers an update of the PR and we currently expect that we can re-use the message from the previous commit: https://github.com/dependabot/dependabot-core/blob/main/common/lib/dependabot/pull_request_updater/github.rb#L186

In this case the original commit was lost so it probably used the description from another commit in the PR?

jeffwidman commented 2 years ago

In this case the original commit was lost so it probably used the description from another commit in the PR?

Yep, that's exactly what happened.

And that code section is what needs updating. But before opening a PR to fix, wanted to make sure we were all in agreement on what the desired behavior here should be.

mctofu commented 2 years ago

I do think the ideal behavior here would have been to have the proper message on the commit. However, I'm seeing that in the spot that we are calling the PR updater internally we don't have the metadata available... We'd need to track down why that is the case and adjust it first.

solvaholic commented 2 years ago

In this case the original commit was lost so it probably used the description from another commit in the PR?

Yep, that's exactly what happened.

And that code section is what needs updating. But before opening a PR to fix, wanted to make sure we were all in agreement on what the desired behavior here should be.

In what scenarios do y'all envision @dependabot would re-use a commit message from a different pull request?

My Ruby fu is weak. Unless Ruby does something surprising with this bit of code, tho, I think any commit commit_being_updated returns should be from the same pull request where @dependabot recreate was run:

            commit =
              commits.find { |c| c.sha == old_commit } ||
              commits.find { |c| c.commit.author.name.include?(author_name) } ||
              commits.first
bdragon commented 2 years ago

As of #5913 we'll now fall back to using the PR title as the commit message if the old commit has been lost. This is an interim solution until we have the capability to recreate the commit message for PR updates (context)

codecholeric commented 8 months ago

I ran into something similar today. Had an action that force-pushed something onto the original Dependabot commit (a version update of the project). However, whenever I tried a @dependabot recreate afterwards it would be incapable of creating a proper commit description and just write Dependabot couldn't find the original pull request head commit, $SHA. I think this is really unfortunate, because the description of @dependabot recreate really sounds like the Dependabot PR will be recreated from scratch. Having this rely on some old commit message that could vanish from the repo is quite surprising.