nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
104.76k stars 28.3k forks source link

Commit queue overwrites commit author #52967

Open targos opened 1 month ago

targos commented 1 month ago

See https://github.com/nodejs/node/pull/52745 and https://github.com/nodejs/node/pull/52860 (@marco-ippolito)

When the commit author and the PR author do not match, the original commit author is lost when the PR is merged.

RedYetiDev commented 1 month ago

I haven't looked at the code for the bot, but theoretically could we just replace the PR author setup with the first commit author?

I'm happy to take a look at it, but I'm not a CODEOWNER, just a triage member

@nodejs/github-bot

aduh95 commented 1 month ago

I think that's a limitation of the GitHub API – the alternatives/workaround are:

I've been trying to do the latter when I notice the commit author is not the same person that sent the PR. I guess we could implement a check to refuse to land a PR when the author will be wrong.

RedYetiDev commented 1 month ago

I guess we could implement a check to refuse to land a PR when the author will be wrong.

That seems (IMO) a good solution, but I'm far from the person who would be affected by any of this

marco-ippolito commented 1 month ago

This changed recently, before when you added the "commit-queue-squash" it would squash on the first commit and keep the author of the first commit as the author of the landed commit. Now the author of the pr becomes the author of the landed commit regardless if the first commit is theirs

targos commented 1 month ago

I made a little experiment using the GH UI: https://github.com/targos/node/pull/18, https://github.com/targos/node/commit/adf665732488a07bf13f25fced06f8b2d987048b Squash and merge indeed changes the author (it doesn't let you keep it, you can only select the email address you want to be associated with the commit). It adds a Co-authored-by: field to the commit message, though.

aduh95 commented 1 month ago

This changed recently, before when you added the "commit-queue-squash" it would squash on the first commit and keep the author of the first commit as the author of the landed commit.

I think you're misremembering, AFAIK it's always been the case that GH uses the "PR opener" as commit author – and we're using the GH API since https://github.com/nodejs/node/pull/40666.

marco-ippolito commented 1 month ago

This changed recently, before when you added the "commit-queue-squash" it would squash on the first commit and keep the author of the first commit as the author of the landed commit.

I think you're misremembering, AFAIK it's always been the case that GH uses the "PR opener" as commit author – and we're using the GH API since #40666.

I remember https://github.com/nodejs/node/pull/45597 maybe it was landed with git node land