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

Backport tools: sort PRs to be cherry picked by merged/closed date #52667

Merged
merged 4 commits into from
Jul 17, 2023

Conversation

ramonjd
Copy link
Member

@ramonjd ramonjd commented Jul 17, 2023

What?

Updates the npm run other:cherry-pick script.

When fetching PRs to be cherry picked, first sort them by pull_request.merged_at (and closed_at if not available) date in ascending order, that is, from the oldest merged/closed PRs to most recently merged/closed PRs.

Before (last_update updated descending)
[
 // This item was the last updated, but not the first to be merged
  {
    id: 1803111783,
    number: 52603,
    title: "Do not autofocus page title field in the 'Draft a new page' modal dialog",
    last_update: 'Mon, 17 Jul 2023 01:28:15 GMT',
    merged: 'Fri, 14 Jul 2023 08:50:31 GMT'
  },
  {
    id: 1801055909,
    number: 52570,
    title: 'Remove "Theme patterns" heading in Pattern library',
    last_update: 'Fri, 14 Jul 2023 19:27:35 GMT',
    merged: 'Fri, 14 Jul 2023 19:27:07 GMT'
  },
  {
    id: 1803789256,
    number: 52617,
    title: 'Use lowercase p in in "Manage Patterns"',
    last_update: 'Fri, 14 Jul 2023 16:21:02 GMT',
    merged: 'Fri, 14 Jul 2023 16:20:37 GMT'
  },
  {
    id: 1803604703,
    number: 52613,
    title: 'Fix Shift+Tab to Block Toolbar',
    last_update: 'Fri, 14 Jul 2023 07:39:10 GMT',
    merged: 'Fri, 14 Jul 2023 04:10:08 GMT'
  },
  {
    id: 1804189935,
    number: 52627,
    title: 'Iframe: Silence style compat warnings when in a BlockPreview',
    last_update: 'Fri, 14 Jul 2023 06:46:41 GMT',
    merged: 'Fri, 14 Jul 2023 05:48:52 GMT'
  },
  {
    id: 1803983317,
    number: 52622,
    title: 'Change password input to type text so contents are visible.',
    last_update: 'Fri, 14 Jul 2023 05:11:01 GMT',
    merged: 'Fri, 14 Jul 2023 05:07:07 GMT'
  },
 // This item was the first to be merged
  {
    id: 1776105739,
    number: 51956,
    title: 'Try restoring the site editor animation',
    last_update: 'Fri, 14 Jul 2023 03:24:24 GMT',
    merged: 'Tue, 04 Jul 2023 09:49:33 GMT'
  }
]
After (sorted by merged ascending)
[
 // This item was the first to be merged
  {
    id: 1776105739,
    number: 51956,
    title: 'Try restoring the site editor animation',
    last_update: 'Fri, 14 Jul 2023 03:24:24 GMT',
    merged: 'Tue, 04 Jul 2023 09:49:33 GMT'
  },
  {
    id: 1803604703,
    number: 52613,
    title: 'Fix Shift+Tab to Block Toolbar',
    last_update: 'Fri, 14 Jul 2023 07:39:10 GMT',
    merged: 'Fri, 14 Jul 2023 04:10:08 GMT'
  },
  {
    id: 1803983317,
    number: 52622,
    title: 'Change password input to type text so contents are visible.',
    last_update: 'Fri, 14 Jul 2023 05:11:01 GMT',
    merged: 'Fri, 14 Jul 2023 05:07:07 GMT'
  },
  {
    id: 1804189935,
    number: 52627,
    title: 'Iframe: Silence style compat warnings when in a BlockPreview',
    last_update: 'Fri, 14 Jul 2023 06:46:41 GMT',
    merged: 'Fri, 14 Jul 2023 05:48:52 GMT'
  },
  {
    id: 1803111783,
    number: 52603,
    title: "Do not autofocus page title field in the 'Draft a new page' modal dialog",
    last_update: 'Mon, 17 Jul 2023 01:28:15 GMT',
    merged: 'Fri, 14 Jul 2023 08:50:31 GMT'
  },
  {
    id: 1803789256,
    number: 52617,
    title: 'Use lowercase p in in "Manage Patterns"',
    last_update: 'Fri, 14 Jul 2023 16:21:02 GMT',
    merged: 'Fri, 14 Jul 2023 16:20:37 GMT'
  },
  {
    id: 1801055909,
    number: 52570,
    title: 'Remove "Theme patterns" heading in Pattern library',
    last_update: 'Fri, 14 Jul 2023 19:27:35 GMT',
    merged: 'Fri, 14 Jul 2023 19:27:07 GMT'
  }
]

Why?

At the moment the script fetches PR according with an "updated" sort flag. These means that the PRs are ordered by the last updated date, with the most-recent item first.

When cherry-picking commits there's always a risk of merge conflicts - hopefully cherry-picking commits from oldest to most recent merge dates will reduce the likelihood of conflicts.

The assumption is that applying newer commits to older ones will better reflect how the current state of the code should be as newer commits may have modified code in the older commits.

Conversely, applying an older commit to a newer commit could lead to merge conflicts where the older commit contains code that has been modified by the newer commit.

How?

Sorting by dates available from the Github API response.

Testing Instructions

You can test against any set of Gutenberg PR labels by running npm run other:cherry-pick "LABEL_NAME"

npm run other:cherry-pick alone will run against the "Backport to WP Beta/RC" label.

To test, I commented out most of the functionality and print out fetched PRs to check the order:

Diff
diff --git a/bin/cherry-pick.mjs b/bin/cherry-pick.mjs
index baf8d42962..f3c4d789dc 100644
--- a/bin/cherry-pick.mjs
+++ b/bin/cherry-pick.mjs
@@ -47,12 +47,14 @@ async function main() {
 	await promptDoYouWantToProceed();
 
 	console.log( `$ git pull origin ${ BRANCH } --rebase...` );
-	cli( 'git', [ 'pull', 'origin', BRANCH, '--rebase' ], true );
+	//cli( 'git', [ 'pull', 'origin', BRANCH, '--rebase' ], true );
 
 	console.log( `$ git fetch origin trunk...` );
-	cli( 'git', [ 'fetch', 'origin', 'trunk' ], true );
+	//cli( 'git', [ 'fetch', 'origin', 'trunk' ], true );
 
 	const PRs = await fetchPRs();
+	console.log( PRs );
+	return;
 	console.log( 'Trying to cherry-pick one by one...' );
 	const [ successes, failures ] = cherryPickAll( PRs );
 	console.log( 'Cherry-picking finished!' );
@@ -136,29 +138,29 @@ async function fetchPRs() {
 	PRs.forEach( ( { number, title } ) =>
 		console.log( indent( `#${ number } – ${ title }` ) )
 	);
-	console.log( 'Fetching commit IDs...' );
-
-	const PRsWithMergeCommit = [];
-	for ( const PR of PRs ) {
-		const { merge_commit_sha: mergeCommitHash } = await GitHubFetch(
-			'/repos/WordPress/Gutenberg/pulls/' + PR.number
-		);
-		PRsWithMergeCommit.push( {
-			...PR,
-			mergeCommitHash,
-		} );
-		if ( ! mergeCommitHash ) {
-			throw new Error(
-				`Cannot fetch the merge commit sha for ${ prToString( PR ) }`
-			);
-		}
-	}
-
-	console.log( 'Done!' );
-	PRsWithMergeCommit.forEach( ( msg ) =>
-		console.log( indent( `${ prToString( msg ) }` ) )
-	);
-	return PRsWithMergeCommit;
+	// console.log( 'Fetching commit IDs...' );
+	//
+	// const PRsWithMergeCommit = [];
+	// for ( const PR of PRs ) {
+	// 	const { merge_commit_sha: mergeCommitHash } = await GitHubFetch(
+	// 		'/repos/WordPress/Gutenberg/pulls/' + PR.number
+	// 	);
+	// 	PRsWithMergeCommit.push( {
+	// 		...PR,
+	// 		mergeCommitHash,
+	// 	} );
+	// 	if ( ! mergeCommitHash ) {
+	// 		throw new Error(
+	// 			`Cannot fetch the merge commit sha for ${ prToString( PR ) }`
+	// 		);
+	// 	}
+	// }
+	//
+	// console.log( 'Done!' );
+	// PRsWithMergeCommit.forEach( ( msg ) =>
+	// 	console.log( indent( `${ prToString( msg ) }` ) )
+	// );
+	return PRs;
 }
 
 /**

@ramonjd ramonjd added the [Type] Build Tooling Issues or PRs related to build tooling label Jul 17, 2023
@ramonjd ramonjd self-assigned this Jul 17, 2023
@ramonjd ramonjd marked this pull request as ready for review July 17, 2023 01:40
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

I've never used this command before myself, so others might have more input, but this looks like a terrific idea to me! And thanks for the helpful test instructions that meant I didn't accidentally auto-comment on a bunch of PRs 😄

✅ Confirmed that this results in the PRs array being sorted by merged_at date from oldest (further back in time of merge date) to most recently merged
✅ Fallback to root closed_at value looks good

LGTM! ✨

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This change should avoid about 90% of merge conflicts during cherry-picking 🎉

@ramonjd
Copy link
Member Author

ramonjd commented Jul 17, 2023

Thanks for reviewing, folks! 🙇🏻

@ramonjd ramonjd merged commit d1f763c into trunk Jul 17, 2023
48 checks passed
@ramonjd ramonjd deleted the try/order-cherry-picked-PRs-by-closed-date branch July 17, 2023 03:19
@github-actions github-actions bot added this to the Gutenberg 16.3 milestone Jul 17, 2023
@tellthemachines
Copy link
Contributor

Let's add this PR to the wp/6.3 branch too so we can use it from there.

ramonjd added a commit that referenced this pull request Jul 17, 2023
…52667)

* Sorting found PRs by closed date in ascending order, from the oldest closed PRs to most recently closed PRs.

* Now checking pull_request?.merged_at

* Now checking pull_request?.merged_at

* semi colon
@ramonjd
Copy link
Member Author

ramonjd commented Jul 17, 2023

I have committed this PR's merge commit d1f763c to the wp/6.3 branch

westonruter added a commit that referenced this pull request Jul 18, 2023
* trunk: (36 commits)
  Use `_get_block_template_file` function and set $area variable. (#52708)
  Change Delete page menu item to Move to trash. (#52641)
  Search block: Enqueue view script through block.json (#52552)
  Patterns: fix bug with Create Patterns menu not showing in site editor page editing (#52671)
  Github workflow: add a PHP backport changes action (#52096)
  Add layout API documentation. (#52673)
  Show uncategorized patterns on the Editor > Patterns page (#52633)
  Patterns: Remove `reusable` text from menu once rename hint has been dismissed (#52664)
  Update locked pattern tooltips (#52497)
  Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save (#52682)
  Use posts instead of template parts for navigation color tests (#52654)
  Site Editor: Don't allow creating template part on the Patterns page for non-block themes (#52656)
  Site Editor: Fix incorrect 'useSelect' usage (#52683)
  Update issue gardening automation with new label (#52173)
  i18n: Make the tab labels of `ColorGradientSettingsDropdown` component translatable (#52669)
  Post Content link color should not be applied to placeholder component links (#52367)
  [Mobile] Update toolbar icons and colors (#52336)
  Avoid copying global style presets via the styles compatibility hook (#52640)
  Show warning on removal of Post Template block in the site editor. (#52666)
  Backport tools: sort PRs to be cherry picked by merged/closed date (#52667)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants