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

git-node: add git-node-release #388

Merged
merged 16 commits into from Apr 3, 2020
Merged

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Feb 29, 2020

This PR constitutes the first portion of an automated release flow to abstract away the tedious aspects of preparing and promoting releases.

The preparation and promotion aspects of this new flow will be split into two separate PRs for easier review, so this is just the preparation step, with the promotion step marked as a TODO for implementation in a subsequent PR.

There are several things i'd like to refactor to make this a little less verbose in the future, but to avoid scope creep for now i've just marked some of them TODO and unless you all feel they are blocking i'll move to address them in follow-ups.

@codebytere codebytere added the Work In Progress PR's that are in progress label Feb 29, 2020
@codebytere codebytere force-pushed the release-session branch 5 times, most recently from c315f3e to 4c5ccfd Compare February 29, 2020 05:40
@codecov
Copy link

codecov bot commented Feb 29, 2020

Codecov Report

Merging #388 into master will increase coverage by 0.29%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
+ Coverage   75.98%   76.28%   +0.29%     
==========================================
  Files          21       21              
  Lines        1445     1480      +35     
==========================================
+ Hits         1098     1129      +31     
- Misses        347      351       +4     
Impacted Files Coverage Δ
lib/pr_checker.js 96.13% <0.00%> (-1.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66037a6...be4c952. Read the comment docs.

@codebytere codebytere force-pushed the release-session branch 6 times, most recently from 9290e3e to 076cd10 Compare February 29, 2020 19:01
@Trott
Copy link
Member

Trott commented Feb 29, 2020

@nodejs/releasers

@codebytere codebytere force-pushed the release-session branch 4 times, most recently from b41c616 to 8863adc Compare February 29, 2020 23:38
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
@targos
Copy link
Member

targos commented Mar 1, 2020

Thanks for working on this!

@codebytere codebytere marked this pull request as ready for review March 1, 2020 17:43
@codebytere codebytere removed the Work In Progress PR's that are in progress label Mar 1, 2020
@codebytere codebytere force-pushed the release-session branch 4 times, most recently from 1210673 to 5ba3eb3 Compare March 1, 2020 19:48
Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

This is incredible work. Lots of thoughts inline.

lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
components/git/release.js Outdated Show resolved Hide resolved
components/git/release.js Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
lib/release.js Outdated Show resolved Hide resolved
@MylesBorins
Copy link
Member

MylesBorins commented Mar 10, 2020

Some bugs I found playing with this

  • I provided the version as v13.11.0 and the branch it crearted was vv13.11.0 (and similar duplication across meta data. Should likely not do that 😅 (ignore v)
  • I accidentally said yes to changing module version number which resulted in reformatting the entire module version data and thus a lot of churn. Might want to make sure that generated module version number information has a consistent style. Honestly the question came as a surprise... and we should maybe only ask it for Semver-Major release (imho)

edit:

was also missing a newline between the new changelog and old changlog (after the list of commits)

@MylesBorins
Copy link
Member

MylesBorins commented Mar 10, 2020

I also just accidentally clicked through on the "create PR" before I had a chance to audit stuff... would prefer to not have that step 😅

edit:

opening the PR failed lol

Error: remote:
remote: Create a pull request for 'v13.11.0-proposal' on GitHub by visiting:
remote:      https://github.com/nodejs/node/pull/new/v13.11.0-proposal
remote:
To github.com:nodejs/node.git
 * [new branch]            v13.11.0-proposal -> v13.11.0-proposal

    at exports.runSync (/Users/mborins/code/node-core-utils/lib/run.js:60:11)
    at ReleasePreparation.openPullRequest (/Users/mborins/code/node-core-utils/lib/prepare_release.js:175:20)
    at ReleasePreparation.prepare (/Users/mborins/code/node-core-utils/lib/prepare_release.js:118:10)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

@MylesBorins
Copy link
Member

MylesBorins commented Mar 10, 2020

One other bit... while it is nice having the notable commit information in the commit message we likely don't want it as markdown

edit:

also noticed that my username is all in lower caps in the changelog

@MylesBorins
Copy link
Member

Automation is missing one of the link references at the top of the version specific changelog

nodejs/node#32218

@MylesBorins
Copy link
Member

dogfooded again. Everything worked great until I attempted to push upstream and then it bailed with the following warning

Error: remote:
remote: Create a pull request for 'v13.12.0-proposal' on GitHub by visiting:
remote:      https://github.com/nodejs/node/pull/new/v13.12.0-proposal
remote:
To github.com:nodejs/node.git
 * [new branch]            v13.12.0-proposal -> v13.12.0-proposal

    at exports.runSync (/Users/mborins/code/node-core-utils/lib/run.js:60:11)
    at ReleasePreparation.pushBranch (/Users/mborins/code/node-core-utils/lib/prepare_release.js:247:5)
    at ReleasePreparation.prepare (/Users/mborins/code/node-core-utils/lib/prepare_release.js:160:10)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

@rvagg
Copy link
Member

rvagg commented Mar 24, 2020

wow, this is a great way to deal with release complexity, and it even edits abi_version_registry!

Minor suggestion is to add changelog-maker to the dependencies list, not strictly essential but since you're directly relying on the executable it it may be a good idea to lock the version to the code rather than whatever branch-diff pulls in.

@MylesBorins
Copy link
Member

Seems like the href for the prior release is getting replaced rather than appended in CHANGELOG_vX.x

nodejs/node#32376 (comment)

@MylesBorins
Copy link
Member

trying again and the "auto detect" wasn't working for me... it kept trying to make a v14.x release and complaining that i was on v13.x (for a 13.x release)

Copy link
Member

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

Based on current usage I've had this LGTM.

I think it makes sense to land this and iterate / update

@targos
Copy link
Member

targos commented Apr 3, 2020

Rubberstamp LGTM. I'll try it for the next release I prepare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants