Directory

"co-authored-by" tags in commits are ignored when collecting props · Issue #86 · WordPress/props-bot-action · GitHub
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

"co-authored-by" tags in commits are ignored when collecting props #86

Open
jrfnl opened this issue Mar 8, 2024 · 3 comments
Open

"co-authored-by" tags in commits are ignored when collecting props #86

jrfnl opened this issue Mar 8, 2024 · 3 comments
Assignees
Labels
[Type] Bug Report Something isn't working

Comments

@jrfnl
Copy link
Member

jrfnl commented Mar 8, 2024

Description

Given:

  • A PR which is opened for review in a repo using the props bot
  • ... and the PR contains one or more commits which already credit contributors using the Co-authored-by: tags.
  • ... and the Co-authored-by: tags follow the format as proscribed in the attribution page, i.e. Co-authored-by: githubusername <dotorgusername@git.wordpress.org>
  • ... and the accounts mentioned are linked correctly (i.e. the link between w.org and GitHub has been established correctly prior to the commits being made)

I'd expect:

The props bot to include those people being acknowledged as co-authors to be included in the props list.

Instead this happened:

Only the committer is included in the props list, co-authors are ignored by the bot.

Version

v1

Workflow file

https://github.com/WordPress/wordpress-develop/blob/trunk/.github/workflows/props-bot.yml

Link

WordPress/wordpress-develop#6235

@jrfnl jrfnl added the [Type] Bug Report Something isn't working label Mar 8, 2024
@desrosj
Copy link
Contributor

desrosj commented Mar 14, 2024

Thanks for this @jrfnl!

Some rough thoughts on this:

  • Co-authored-by trailers in commits are not currently parsed. Thinking this through, it seems that they should be parsed, but only when collecting an list of contributors for SVN because a full list is needed when committing to develop.svn.wordpress.org.
  • If the intention is to merge the PR in Git, then the squash and merge message should include the commit messages for each commit in the PR and thus any Co-authored-by trailers from those commits. These would be parsed out with any that are added manually.
  • If we want all of the Co-authored-by trailers to be included at the bottom of a merge commit message, then we'd need to advise maintainers to collect them manually. It does not seem like there is a way to configure GitHub to do this in some way.
  • This may actually be preferred, though. For example, say there are 3 commits in a PR branch and contributor A is noted as a co-author in all of them. Contributor A also comments on the PR so is included in the merge commit list produced by the bot. This would result in them receiving 4 props for the PR instead of 1, which is traditionally how we handle that in Core SVN.

I think parsing commit messages and including any trailers for SVN is a good first step here. The Git aspects need more feedback.

@jeffpaul
Copy link
Member

For cases where the PR will be merged in GitHub, it seems like those co-authors-on-commits would still appear in the gitlog and thus be properly credited. It would be nice to have those co-authors-on-commits gathered and presented in its own listing so that whoever is merging the PR could see that list and perhaps include them in the PR merge commit if they wanted or alternatively if those same accounts are also included in props bot listing then they could be ignored in the merge commit since they'd already be part of the gitlog history.

More importantly, and I think @desrosj gets at this, for cases where the PR is not merged into GitHub and is instead committed to SVN, then having those co-authors-on-commits referenced by the props bot for the SVN listing would be desired to ensure those folks are not missed on the SVN commit.

@jrfnl
Copy link
Member Author

jrfnl commented Mar 14, 2024

for cases where the PR is not merged into GitHub and is instead committed to SVN, then having those co-authors-on-commits referenced by the props bot for the SVN listing would be desired to ensure those folks are not missed on the SVN commit.

That is exactly the usecase for which I opened the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Report Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants