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.

Sign up for GitHub

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

Reformat the Props Bot comment #21

Merged
merged 18 commits into from
Jan 24, 2024
Merged

Reformat the Props Bot comment #21

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
dbd7ca5
Reformat the Props Bot comment.
desrosj Jan 18, 2024
9bfdbf4
Some cleanup and typo fixes.
desrosj Jan 18, 2024
3c9cb48
Only show `Unlinked contributor:` when there's 1 or more.
desrosj Jan 18, 2024
6df5feb
More formatting corrections.
desrosj Jan 18, 2024
5f7e6b7
Remove some debug code.
desrosj Jan 18, 2024
bd1c7ce
Don't recommend the merging contributor remove themselves.
desrosj Jan 18, 2024
a2b9c88
Breaking out the pinking shears.
desrosj Jan 18, 2024
8c631f2
Add note about `props-bot` label.
desrosj Jan 18, 2024
ee2caed
Remove stray semi-colon.
desrosj Jan 18, 2024
a74a6c2
More refinement of phrasing.
desrosj Jan 18, 2024
b212ae0
Avoid including a subscript in comment.
desrosj Jan 18, 2024
03d60a3
Remove extra `core.debug()`.
desrosj Jan 18, 2024
93260ce
Remove correct extra debug line.
desrosj Jan 18, 2024
5ca2d5e
Change phrasing.
desrosj Jan 19, 2024
dae2833
Rebuilding dist.
desrosj Jan 19, 2024
b5e878f
Tweak messaging.
desrosj Jan 22, 2024
9d62018
Update built files.
desrosj Jan 22, 2024
124373b
Merge branch 'trunk' into update/comment-format
desrosj Jan 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 57 additions & 22 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 20 additions & 16 deletions src/contribution-collector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export async function getContributorsList() {
.forEach((comment) => contributors.commenters.add(comment.author.login));

core.debug('Commenters:');
core.debug(contributors);
core.debug(contributors.commenters);

// Process reporters and commenters for linked issues.
Expand Down Expand Up @@ -162,8 +163,13 @@ export async function getContributorsList() {
core.debug(githubUsers);
}

// List to return from the function.
const contributorLists = [];
contributorLists['github'] = [];

// Collect WordPress.org usernames
const wpOrgData = await getWPOrgData(githubUsers);
contributorLists['svn'] = [];

core.debug('WordPress.org raw data:');
core.debug(wpOrgData);
Expand All @@ -175,28 +181,25 @@ export async function getContributorsList() {
wpOrgData[contributor] !== false
) {
userData[contributor].dotOrg = wpOrgData[contributor].slug;
contributorLists['svn'].push(wpOrgData[contributor].slug);
}
});

return contributorTypes
contributorLists['coAuthored'] = [];
contributorLists['unlinked'] = [];

contributorTypes
.map((priority) => {
// Skip an empty set of contributors.
if (contributors[priority].length === 0) {
return [];
}

// Add a header for each section.
const header =
"# " + priority.replace(/^./, (char) => char.toUpperCase()) + "\n";

// Generate each props entry, and join them into a single string.
return (
header +
[...contributors[priority]]
.map((username) => {
if ('unlinked' == priority) {
core.debug( 'Unlinked contributor: ' + username );
return `Unlinked contributor: ${username}`;
return;
}

const { dotOrg } = userData[username];
Expand All @@ -206,17 +209,18 @@ export async function getContributorsList() {
"dotOrg"
)
) {
contributors.unlinked.add(username);
contributorLists['unlinked'].push(username);
return;
}

return `Co-Authored-By: ${username} <${dotOrg}@git.wordpress.org>`;
return contributorLists['coAuthored'].push( `Co-Authored-By: ${username} <${dotOrg}@git.wordpress.org>` );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to obtain the GitHub no-reply email addresses via the WP API you're using, ie in the format [email protected]? This will allow co-authored commits to show up without people needing to associate it with their account. As an example, mine is [email protected]

Copy link
Contributor Author

@desrosj desrosj Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but because our ultimate goal is to give credit in the Credits API, I chose to use the [email protected] format. I also didn't love the idea of hard coding personal emails in the commit messages if someone has that setting turned off. I don't recall if the anonymous format worked even if you had that disabled. I could test if there's a strong preference to use that format instead.

The other thought I had was for consistency with how committers are attributed credit from the WordPress/wordpress-develop mirror. When a contributor is granted commit access, they are told to add the @git.wordpress.org format to their account, as that's the email given to the commit author. A "pie in the sky" idea that I'll investigate once we get this initial round settled is to include Co-Authored-By trailers for everyone that receives Props in an SVN commit. My thinking there is that the same format as the committer would make sense.

By including unlinked contributors in the commit message, though, we can easily extract their GH slug from the list and look in the .org database for any contributors that have connected their account since any of their contributions were merged.

There's some weirdness to detail around those emails as noted on that page:

If you created your account on GitHub.com after July 18, 2017, your noreply email address for GitHub is an ID number and your username in the form of [email protected]. If you created your account on GitHub.com prior to July 18, 2017, and enabled Keep my email address private prior to that date, your noreply email address from GitHub is [email protected].
In my research from a year or so ago, I couldn't find a way to figure out which format a specific user has. But in recent testing, I found that both formats worked, regardless of the format it tells you you have.

Another layer to that is on the Change your username page:

If you've been using a GitHub-provided private commit email address, whether or not your commit history will be retained after an account rename depends on the format of the email address. Git commits that are associated with your GitHub-provided noreply email address won't be attributed to your new username and won't appear in your contributions graph, unless your noreply email address is in the form of [email protected]. Older versions of the noreply email address that do not contain a numeric ID will not be associated with your GitHub account after changing your username.

So we'd always need to use the ID+USERNAME format just in case someone changes their username. On the other hand, WP.org usernames cannot be changed.

Sorry for the brain dump, but this was a good chance to document some of my thinking/findings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the goal here is to improve the breadth of folks properly credited from GitHub repos back to WordPress core major releases and to streamline the major release credits gathering process, the approach that @desrosj has here makes sense. If the goal was to ensure all GitHub users were properly credited on PRs, then using the format @peterwilsoncc noted would likely be the best approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been thinking about this the last few days.

I think I do prefer using all .org usernames, but it could be reasonable to use the GH format email for any contributor that's not linked to a .org one.

We can still parse out their GH user and look them up in the .org API, and they'd receive attribution on GH from their anonymous email. But that also removes some of the incentive to connect their account in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@desrosj Does the WP.org integration add [email protected] to a users profile when they link their accounts or is that a step the contributor would need to do to get recognition on GitHub?

Copy link
Contributor Author

@desrosj desrosj Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, they will need to add it manually. I plan to include this in the announcement post, and the documentation. Based on this comment , though, it doesn't seem that our authentication request currently allows us to modify your profile at all. We'd need to add that to the request, and contributors with pre-existing connections would need to re-approve the connection with the new permissions.

I was talking this through with @dd32 last night. Wanted to document our discussion. At first he did not like using @git.wp.org emails, but after some discussion, this was how he summarized his thoughts:

  • On a GitHub-first repo, it makes sense to use @…github.com emails
  • On a SVN repo, it makes sense to use username
  • On a SVN -> Git sync, it makes sense to append Co-Authored-by: …@git.wordpress.org (But who knows if we wanna do that)
  • On a SVN -> Git -> GitHub sync, We can’t modify the commit message of ^ so meh, i guess it makes sense to use @git.w.org
  • Because of ^, it makes sense that on a GitHub-First repo, we could just use @git.w.org for consistency.

It's worth noting that in that list, there are currently no SVN -> Git syncs that do not make it over to GitHub.

Some other takeaways from our discussion:

  • Even though we're heavily using GitHub at this point, there's always a non-0 chance that as a project we decide to use something other than GitHub (ie. GitLab). If this does happen, then the ID+username@github emails would be out of place. We could likely add a way for the tool to support these, but using wp.org ones would be somewhat tool-agnostic.
  • There's currently no ability to validate that you own the @git.wp.org email that represents you. Adding the ability to verify this through email will likely be appropriate to avoid any problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone and created #28 for changing the unlinked contributor list to use GH emails. I think that makes sense, but figured it would be good to tackle separately.

})
.filter((el) => el)
.join("\n")
);
})
.join("\n\n");
.filter((el) => el);
});

core.debug( contributorLists );

return contributorLists;
}

/**
Expand Down
43 changes: 37 additions & 6 deletions src/github.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,19 @@ export default class GitHub {
* - If a comment already exists, it will be updated.
*
* @param {Object} context The GitHub context.
* @param {string} contributorsList The list of contributors.
* @param {array} contributorsList The list of contributors.
*/
async commentProps({ context, contributorsList }) {
if (!contributorsList) {
core.info("No contributors list provided.");
core.info("No contributors were provided.");
return;
}

core.debug( "Contributor list received:" );
core.debug( contributorsList );
core.debug( contributorsList.svn );
console.debug( contributorsList );

let prNumber = context.payload?.pull_request?.number;
if ( 'issue_comment' === context.eventName ) {
prNumber = context.payload?.issue?.number;
Expand All @@ -133,11 +138,37 @@ export default class GitHub {
issue_number: prNumber,
};

const commentMessage =
"Here is a list of everyone that appears to have contributed to this PR and any linked issues:\n\n" +
let commentMessage = "Hello contributors!\n\n" +
"I've collected a list of people who have interacted in some way with this pull request or any linked issues. There's a few useful formats included below, and I'll continue to update this list as activity occurs.\n\n";

if ( contributorsList['unlinked'].length > 0 ) {
commentMessage += "## Unlinked Accounts\n\n" +
"It appears there are some GitHub contributors participating here that have not linked their WordPress.org accounts.\n\n" +
"@" + contributorsList['unlinked'].join(', @') + ": Thank you for your contribution to this repository!\n\n" +
"The WordPress project gives contributors attribution through the [WordPress.org Credits API](https://api.wordpress.org/core/credits/1.1/). However, attribution can only be given to a WordPress.org account.\n\n" +
"Please take a moment to connect your GitHub and WordPress.org accounts when you have a moment so that your contribution can be properly recognized. You'll find [step by step instructions on the Making WordPress Core blog](https://make.wordpress.org/core/2020/03/19/associating-github-accounts-with-wordpress-org-profiles/).\n\n";
}

commentMessage += "## Core SVN\n\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@desrosj, would it be possible to make this configurable? While Core SVN makes perfect sense in WordPress develop, the GitHub Merge isn't that useful. Then, when contributing to Gutenberg, it exactly the other way around. So while the default could be as is, what if you could pass the setting when defining the GitHub workflow?

svnProps: true
gitCoAuthored: true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make an issue for this on the @WordPress/props-bot-core repo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pleasure: #38 :)

"If you're a Core Committer, use this list when committing to `wordpress-develop` in SVN:\n" +
"```\n" +
"Props: " + contributorsList['svn'].join(', ') + "." +
"\n```\n\n" +
"## GitHub Merge commits\n\n" +
"If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.\n\n" +
"```\n" +
contributorsList +
"\n```";
"Unlinked contributors: " + contributorsList['unlinked'].join(', ') + ".\n\n" +
contributorsList['coAuthored'].join("\n") +
"\n```\n\n" +
"**Important notes**:" +
"- Giving props is mandatory for this repository if you are a project maintainer or committer**.\n\n" +
"- The list of `Co-Authored-By:` trailers must be preceded by a blank line.\n" +
"- Usernames must not start with an `@`.\n" +
"- Nothing can come after the `Co-Authored-By:` trailers.\n" +
"- Please include the list of unlinked contributors. If they do connect their GitHub and WordPress.org accounts in the future, this contribution can be credited to them later.\n" +
"- Merging contributors should remove themselves from the list. As the merging contributor, props will be given for being the author of the merge commit.\n" +
"- As always, please manually review this list. [Give props liberally](https://make.wordpress.org/core/handbook/best-practices/commit-messages/#props), but remove anyone users who spammed or did not contribute positively.\n" +
"- If you're unsure, please ask in the [#core-committers channel in Slack](https://wordpress.slack.com/archives/C18723MQ8).\n";

const comment = {
...commentInfo,
Expand Down
Loading