Add support for auto-generating multiple RCs for the current version if a previous RC exists #25971

fullofcaffeine · 2020-10-09T02:40:47Z

Description

Corresponding issue: #25970

Latest update and test results here: #25971 (comment).

Add support for auto-generating multiple RCs or the current version if a previous RC exist.

If a previous RC exists, it will generate a new RC, in sequence. If not, it will fallback to the regular logic of generating an RC1 for the nextMajor version, as well as creating the new releaseBranch.

How has this been tested?

I've run it locally and changed the config to point to my fork of GB.

To test it out, change the values of https://github.com/WordPress/gutenberg/blob/master/bin/plugin/config.js to point to a test fork of Gutenberg. Example.

First, run ./bin/plugin/cli.js rc from Gitenberg's root dir. This assumes the last release is a stable non-RC one. The behaviour here should be the same as before the changes here.
Now, you can take note of where the script cloned the version of GB downloaded when you ran the script in 1), and then manually set this var to point to its path: https://github.com/fullofcaffeine/gutenberg/blob/add/support-multiple-rcs-release-tool/bin/plugin/commands/release.js#L719 (i.e: const gitWorkingDirectory = '/tmp/59506893-74ff-4012-9486-36a0c7ca2869';)
It will then release RC1 for the next major version;
Run the script again and it should pick up the current version and will automatically increment it to RC2.
Run ./bin/plugin/cli.js stable to release a stable version.
Run it again to release a point release.

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

bin/plugin/commands/release.js

ockham

Left a few notes. Looking pretty good overall!

ockham · 2020-10-12T12:46:26Z

We should also update this line:

gutenberg/bin/plugin/cli.js

Line 34 in d4a74cc

'Release an RC version of the plugin (supports only rc.1 for now)'

bin/plugin/cli.js

bin/plugin/commands/release.js

ockham

LGTM, and tests well (as far as I could test).

Let's wait for tests to go green, and ideally someone who's more familiar with these tools to approve 👍

cc/ @youknowriad @gziolo @mcsf

fullofcaffeine · 2020-10-14T00:40:01Z

bin/plugin/commands/release.js

@@ -390,9 +387,11 @@ async function runPushGitChangesStep(
 				true,
 				abortMessage
 			);
+			// We need to force-push because we are effectivelly rebasing from master


Since the release commit is cherry-picked into master, the SHA changes, thus, pushing to the release branch is not possible anymore without force-pushing in the case of subsequent RCs.

This seems to work well, but I don't know if there might be unforeseen side-effects to force-pushing to the release branch. I'd love some feedback here.

I don't like this. we shouldn't be force pushing to master IMO, we could potentially lose merges that happened in the meantime.

At the very least, this should be --force-with-lease.

(Btw, everyone should use that option in their daily Git interactions — I recommend adding aliases to Git like fpush = push --force-with-lease or fp or pf or whatever.)

Our ideal scenario here would be to have all RC commits (alongside potential other, cherry-picked, ones) on the release branch, no? So we need to ensure that we start off from the point where we "left off" with the previous RC. I'm wondering what's currently happening that gets in the way of that?

Could it be that the git.checkoutLocalBranch call in runReleaseBranchCreationStep should only be called if the branch didn't exist before, and that we need to run checkoutRemoteBranch otherwise?

Since the release commit is cherry-picked into master, the SHA changes, thus, pushing to the release branch is not possible anymore without force-pushing in the case of subsequent RCs.

Are you sure that's the reason? 🤔 Wouldn't we run into the same problem when releasing a stable version after an RC?

I'm starting to think it would make sense to change the criterion for the ternary that runs either runReleaseBranchCreationStep or runReleaseBranchCheckoutStep from isRC to isPackageVersionRC, since it captures better what we're trying to do here:

If the package.json version is an RC, then we can assume that a release branch has been created.

If it doesn't, we're at the start of a new release cycle and need to create that branch.

This will mean moving some of the version computing logic around, but I think it'll fall in place quite naturally.

@ockham that makes sense since second/third RCs will just just use the existing branch with cherrypicked commits

I don't like this. we shouldn't be force pushing to master IMO, we could potentially lose merges that happened in the meantime.

Note that the force push here is not to master, but to the releaseBranch only.

Are you sure that's the reason? thinking Wouldn't we run into the same problem when releasing a stable version after an RC?

To be honest, I'm not 100% sure, that looked like the reason and I found this workaround. I'll review this today.

fullofcaffeine · 2020-10-14T02:49:23Z

bin/plugin/lib/git.js

 	const simpleGit = SimpleGit( gitWorkingDirectoryPath );
-	await simpleGit.push( 'origin', branchName );
+
+	// TODO The type definitions for simple-git are wrong and will complain here.


As the comment states, there seems to be a mismatch between the TS typing declarations and the js implementation of the push method in simple-git. I tried passing the options array as the last arg as the type docs say, but it didn't work.

After inspecting the actual code, I realized thet last param is called "then" and is unused. By checking the implementation, I saw that the right way to pass these options was to pass them as the first argument.

~~I'll look into it again tomorrow.~~

I managed to fix this by passing the options as an object instead of an array (as the last argument). The interface here is not the most intuitive ¯_(ツ)_/¯

fullofcaffeine · 2020-10-14T03:42:29Z

This seems to work well. I just tested twice in order to get RC1 and the subsequent RC2:

Output for RC1: https://gist.github.com/fullofcaffeine/848e637f16c0b856766185620d9574ee
Output for RC2: https://gist.github.com/fullofcaffeine/d0a2880ac430bc60d40d67547faee24c

I deleted the release for those while testing, but here's as sample release I just created by releasing RC3:

https://github.com/fullofcaffeine/gutenberg/releases/tag/v9.2.0-rc.3

I don't know why it's still a prerelease though, even after finishing the whole script. Is that expected?

youknowriad · 2020-10-14T07:49:50Z

This is cool, aside the force push which I think is very risky, this is looking good.

The pre-release is normal, we mark all "RCs" as "pre-releases" explicitly in the script.

mcsf · 2020-10-14T10:44:28Z

bin/plugin/commands/release.js

+			log(
+				'>> The local release branch ' +
+					formats.success( releaseBranch ) +
+					' has been successfully created.'
+			);


Shouldn't this remain after the call to git.createLocalBranch?

Good point. The call to createLocalBranch has been moved down becuase I found out that was also needed for the RC scenario, as it essentially checkout th branch. It doesn't make sense indeed to show a confirmation message before the actual thing happens :)

mcsf · 2020-10-14T10:46:17Z

bin/plugin/commands/release.js

@@ -390,9 +387,11 @@ async function runPushGitChangesStep(
 				true,
 				abortMessage
 			);
+			// We need to force-push because we are effectivelly rebasing from master


At the very least, this should be --force-with-lease.

(Btw, everyone should use that option in their daily Git interactions — I recommend adding aliases to Git like fpush = push --force-with-lease or fp or pf or whatever.)

fullofcaffeine · 2020-10-15T00:57:12Z

Thanks @ockham for lending your hand today and to help me to understand the underlying issue (that led me to try the force-push approach*) 🙇🏻

I had to re-arrange the logic slightly again as it was failing just when trying to update from stable to stable. I tried to simplify it a bit, too.

I did another round of tests (using my own fork of GB, see config here) and it now seems to work well (no more force-push needed). Here are the scenarios and their respective results:

command	package version	new version	observations
rc	`9.1.1`	`9.2.0-rc.1`	Uses this function to generate the next version
rc	`9.2.0-rc.1`	`9.2.0-rc.2`	increments the RC ad infinitum until a stable version is released
stable	`9.2.0-rc.2`	`9.2.0`	increments the patch version, effectively removing the RC portion
stable	`9.2.0`	`9.2.1`	increments the patch version ad infinitum until we release another RC*

Note that I didn't fully finish the whole process when running the stable command, I replied 'N' when it got to the SVN part, but I don't think it's needed to consider these tests as succesful, as all the relevant steps have been run prior to it.

*Basically, we were trying to create a branch and push in place of checking out the existing branch for the scenarios that created a release in the same major/minor version. We had to adjust the logic to be aware of that, too.

*This effectively makes the rc command mandatory for new minor/major versions.

@youknowriad @gziolo @mcsf Could you have another look? 🙇🏻

…f a previous RC exists If a previous RC exists, it will generate a new RC, in sequence. If not, it will fallback to the regular logic of generating a RC1 for the `nextMajor` version, as well as creating the new `releaseBranch`.

Improve variable naming, simplify the code.

… new behavior of releaseing multiple RCs for the same version

Co-authored-by: Bernie Reiter <ockham@raz.or.at>

… it is needed We need to parse twice, first the version from the package to be able to use the `prerelease` property so we can decide how to proceed. Then after we know if the current version is an RC or not, we parse the next version so that the releaseBranch can be defined.

…C releases Since the release is cherry-picked into master, the SHA changes, thus, pushing to the release branch is not possible anymore without force-pushing.

This method has a very confusing signature. The comment in the source-code suggests you can pass all arguments as an options array as part of the 1st argument (remote) and that's what I was doing. However, it's not compatible with the type definitions. You can't pass an array of options as the last arguments as it will just swallow them silently. For the last argument to be recognized as git options, you should pass it as a JS object. If the options does not accept arguments, its value should be null. Changing it to an object fixed the type errors too.

Be more explicit, fix case where stable -> stable would try to create a branch instead of checking out the existing for the version.

bin/plugin/commands/release.js

ockham

Thanks, this is testing well for me now 👍

ockham · 2020-10-16T19:24:10Z

@youknowriad @mcsf Could we get your final blessing please? 🙏

youknowriad · 2020-10-20T07:58:05Z

Ship it, thanks for your work here :)

As of #25971, the release process does support releasing multiple RCs.

ockham mentioned this pull request Oct 12, 2020

[Release Tool] Add support for creating multiple RCs for the same version #25970

Closed

ockham reviewed Oct 12, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

ockham reviewed Oct 12, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

ockham reviewed Oct 12, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

ockham reviewed Oct 12, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

ockham reviewed Oct 12, 2020

View reviewed changes

fullofcaffeine marked this pull request as ready for review October 12, 2020 23:49

fullofcaffeine requested review from ajitbohra, nerrad and ntwb as code owners October 12, 2020 23:49

ockham reviewed Oct 13, 2020

View reviewed changes

bin/plugin/cli.js Outdated Show resolved Hide resolved

ockham requested review from youknowriad, mcsf and gziolo October 13, 2020 10:08

ockham reviewed Oct 13, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

ockham approved these changes Oct 13, 2020

View reviewed changes

fullofcaffeine commented Oct 14, 2020

View reviewed changes

mcsf reviewed Oct 14, 2020

View reviewed changes

fullofcaffeine and others added 7 commits October 14, 2020 23:09

Use semver to detect if its prerelease and to incremenet RCs

2f86bd0

Improve variable naming, simplify the code.

Update the description for the release-plugin-rc command to match the…

532cdef

… new behavior of releaseing multiple RCs for the same version

Simplify the command description

77f1a44

Co-authored-by: Bernie Reiter <ockham@raz.or.at>

Whitespace

8b984b6

Force-push to the release branch to avoid ref errors in sub-sequent R…

366a748

…C releases Since the release is cherry-picked into master, the SHA changes, thus, pushing to the release branch is not possible anymore without force-pushing.

fullofcaffeine and others added 5 commits October 14, 2020 23:09

Simplify jsdoc type

8e411cc

Re-arrange version arithmetics

98d4789

Add back exports :rolleyes:

b4ccbc7

Re-arrange the version versioning logic a bit

069f5f8

Be more explicit, fix case where stable -> stable would try to create a branch instead of checking out the existing for the version.

ockham reviewed Oct 15, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

ockham reviewed Oct 15, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

ockham reviewed Oct 15, 2020

View reviewed changes

bin/plugin/commands/release.js Outdated Show resolved Hide resolved

fullofcaffeine changed the title ~~Add support for auto-generating multiple RCs or the current version if a previous RC exists~~ Add support for auto-generating multiple RCs for the current version if a previous RC exists Oct 15, 2020

Remove redundant version return and DRY things a bit more

fe8dd47

ockham approved these changes Oct 16, 2020

View reviewed changes

simison merged commit 0adfef9 into WordPress:master Oct 20, 2020

github-actions bot added this to the Gutenberg 9.3 milestone Oct 20, 2020

vcanales added a commit that referenced this pull request Oct 21, 2021

remove consecutive rc warning

150556e

As of #25971, the release process does support releasing multiple RCs.

vcanales mentioned this pull request Oct 21, 2021

Release Docs: Remove consecutive RCs warning #35855

Merged

mkaz pushed a commit that referenced this pull request Oct 21, 2021

remove consecutive rc warning (#35855)

ff468f2

As of #25971, the release process does support releasing multiple RCs.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for auto-generating multiple RCs for the current version if a previous RC exists #25971

Add support for auto-generating multiple RCs for the current version if a previous RC exists #25971

fullofcaffeine commented Oct 9, 2020 •

edited

Loading

ockham left a comment

ockham commented Oct 12, 2020

ockham left a comment

fullofcaffeine Oct 14, 2020 •

edited

Loading

youknowriad Oct 14, 2020

mcsf Oct 14, 2020

ockham Oct 14, 2020

ockham Oct 14, 2020

ockham Oct 14, 2020

youknowriad Oct 14, 2020

fullofcaffeine Oct 14, 2020 •

edited

Loading

fullofcaffeine Oct 14, 2020

fullofcaffeine Oct 14, 2020 •

edited

Loading

fullofcaffeine Oct 14, 2020

fullofcaffeine commented Oct 14, 2020 •

edited

Loading

youknowriad commented Oct 14, 2020

mcsf Oct 14, 2020

fullofcaffeine Oct 14, 2020 •

edited

Loading

mcsf Oct 14, 2020

fullofcaffeine commented Oct 15, 2020 •

edited

Loading

ockham left a comment

ockham commented Oct 16, 2020

youknowriad commented Oct 20, 2020

Add support for auto-generating multiple RCs for the current version if a previous RC exists #25971

Add support for auto-generating multiple RCs for the current version if a previous RC exists #25971

Conversation

fullofcaffeine commented Oct 9, 2020 • edited Loading

Description

How has this been tested?

Types of changes

Checklist:

ockham left a comment

Choose a reason for hiding this comment

ockham commented Oct 12, 2020

ockham left a comment

Choose a reason for hiding this comment

fullofcaffeine Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullofcaffeine Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullofcaffeine Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullofcaffeine commented Oct 14, 2020 • edited Loading

youknowriad commented Oct 14, 2020

Choose a reason for hiding this comment

fullofcaffeine Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fullofcaffeine commented Oct 15, 2020 • edited Loading

ockham left a comment

Choose a reason for hiding this comment

ockham commented Oct 16, 2020

youknowriad commented Oct 20, 2020

fullofcaffeine commented Oct 9, 2020 •

edited

Loading

fullofcaffeine Oct 14, 2020 •

edited

Loading

fullofcaffeine Oct 14, 2020 •

edited

Loading

fullofcaffeine Oct 14, 2020 •

edited

Loading

fullofcaffeine commented Oct 14, 2020 •

edited

Loading

fullofcaffeine Oct 14, 2020 •

edited

Loading

fullofcaffeine commented Oct 15, 2020 •

edited

Loading