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

Commit queue overwrites commit author #52967

Open
targos opened this issue May 13, 2024 · 7 comments
Open

Commit queue overwrites commit author #52967

targos opened this issue May 13, 2024 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. meta Issues and PRs related to the general management of the project.

Comments

@targos
Copy link
Member

targos commented May 13, 2024

See #52745 and #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.

@avivkeller avivkeller added confirmed-bug Issues with confirmed bugs. meta Issues and PRs related to the general management of the project. labels May 13, 2024
@avivkeller
Copy link
Member

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
Copy link
Contributor

aduh95 commented May 14, 2024

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

  • land the PR with commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. (i.e. no purple merge)
  • land the PR manually
  • add the original author as a co-author

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.

@avivkeller
Copy link
Member

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
Copy link
Member

marco-ippolito commented May 15, 2024

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
Copy link
Member Author

targos commented May 15, 2024

I made a little experiment using the GH UI: targos#18, targos@adf6657
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
Copy link
Contributor

aduh95 commented May 15, 2024

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.

@marco-ippolito
Copy link
Member

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 #45597 maybe it was landed with git node land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

No branches or pull requests

4 participants