The Wayback Machine - https://web.archive.org/web/20201221003739/https://github.com/nodejs/node-core-utils/pull/402
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 release promotion step #402

Open
wants to merge 12 commits into
base: master
from

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Apr 9, 2020

Refs #388.

Some of this hard to test, since it involved taking real steps that we take during releases.

I've chosen to seek active confirmation more here than in the prep stage, since many of the steps are irreversible, but I'm open to any and all thoughts about how much we might want.

This adds the secondary portion of release automation, for the promotion step. Specifically, we want to:

  1. Verify that the release PR has green CI and an approval
  2. Create and sign the release tag
  3. Set up for next release
  4. Merge the release proposal branch into the release branch
  5. Cherry pick release commit to master
  6. Push release tag
  7. Promote and sign the release builds

cc @nodejs/releasers

@codebytere codebytere force-pushed the codebytere:git-node-release-promote branch 2 times, most recently from b44d683 to 84b0a27 Apr 9, 2020
@codecov
Copy link

@codecov codecov bot commented Apr 9, 2020

Codecov Report

Merging #402 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #402   +/-   ##
=======================================
  Coverage   76.34%   76.34%           
=======================================
  Files          21       21           
  Lines        1484     1484           
=======================================
  Hits         1133     1133           
  Misses        351      351           

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 f58051c...3377b44. Read the comment docs.

@targos
Copy link
Member

@targos targos commented Apr 9, 2020

I'll review and try this when I do the 13.x release next week

@codebytere codebytere force-pushed the codebytere:git-node-release-promote branch 3 times, most recently from 0666c26 to 13191bd Apr 9, 2020
@codebytere codebytere marked this pull request as ready for review Apr 9, 2020
@targos
Copy link
Member

@targos targos commented Apr 13, 2020

I'm trying git-node release --prepare but it runs out of memory while "Updating CHANGELOG_V13.md".

@targos
Copy link
Member

@targos targos commented Apr 14, 2020

Is it supposed to ask me for my GitHub credentials?
It blocks immediately at:

$ /home/mzasso/git/nodejs/node-core-utils/bin/git-node release --promote 32813 
⠋ Verifying Releaser statusIf this is your first time running this command, follow the instructions to create an access token. If you prefer to create it yourself on Github, see https://github.com/nodejs/node-core-utils/blob/master/README.md.
⠸ Verifying Releaser status
@targos
Copy link
Member

@targos targos commented Apr 14, 2020

Okay, I found that I can enter my username/password/otp code and it continues.
Then:

⠴ Verifying Releaser status
✔  Received member information of nodejs/releasers
✔  undefined is not a Releaser; aborting release
@targos
Copy link
Member

@targos targos commented Apr 14, 2020

The output of the next steps is weird:

✔  Done loading data for nodejs/node/pull/32813
... skipped some lines
   ✔  Approvals: 6
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/32813#pullrequestreview-393019854
   ✔  - Colin Ihrig (@cjihrig) (TSC): https://github.com/nodejs/node/pull/32813#pullrequestreview-393040785
   ✔  - Richard Lau (@richardlau): https://github.com/nodejs/node/pull/32813#pullrequestreview-393075642
   ✔  - Shelley Vohr (@codebytere) (TSC): https://github.com/nodejs/node/pull/32813#pullrequestreview-393107238
   ✔  - Jiawen Geng (@gengjiawen): https://github.com/nodejs/node/pull/32813#pullrequestreview-393109215
   ✔  - Matheus Marchini (@mmarchini) (TSC): https://github.com/nodejs/node/pull/32813#pullrequestreview-393134711
   ✖  This PR needs to wait 19 more hours to land
✖  Jenkins CI is failing for #32813
--------------------------------------------------------------------------------
? Do you want to proceed? Yes
✔  Jenkins CI is passing
✖  GitHub CI is failing for #32813
--------------------------------------------------------------------------------
? Do you want to proceed? Yes
✔  GitHub CI is passing
✖  #32813 does not have sufficient approvals
--------------------------------------------------------------------------------
? Do you want to proceed? Yes
✔  #32813 has necessary approvals
@targos
Copy link
Member

@targos targos commented Apr 14, 2020

Then it crashes on the secure tag step:

? Tag and sign the release? Yes
Error: /home/mzasso/git/nodejs/node-core-utils/node_modules/git-secure-tag/lib/git-secure-tag/batch.js:83
    throw new Error(`Unexpected reply for ${object}`);
    ^

Error: Unexpected reply for 
    at Batch.getEntry (/home/mzasso/git/nodejs/node-core-utils/node_modules/git-secure-tag/lib/git-secure-tag/batch.js:83:11)
    at Batch.onMissing (/home/mzasso/git/nodejs/node-core-utils/node_modules/git-secure-tag/lib/git-secure-tag/batch.js:89:22)
    at Batch.onHeader (/home/mzasso/git/nodejs/node-core-utils/node_modules/git-secure-tag/lib/git-secure-tag/batch.js:73:10)
    at Batch.onData (/home/mzasso/git/nodejs/node-core-utils/node_modules/git-secure-tag/lib/git-secure-tag/batch.js:56:12)
    at Socket.<anonymous> (/home/mzasso/git/nodejs/node-core-utils/node_modules/git-secure-tag/lib/git-secure-tag/batch.js:17:44)
    at Socket.emit (events.js:315:20)
    at addChunk (_stream_readable.js:297:12)
    at readableAddChunk (_stream_readable.js:273:9)
    at Socket.Readable.push (_stream_readable.js:214:10)
    at Pipe.onStreamRead (internal/stream_base_commons.js:186:23)

    at exports.runSync (/home/mzasso/git/nodejs/node-core-utils/lib/run.js:60:11)
    at ReleasePromotion.secureTagRelease (/home/mzasso/git/nodejs/node-core-utils/lib/promote_release.js:246:12)
    at ReleasePromotion.promote (/home/mzasso/git/nodejs/node-core-utils/lib/promote_release.js:85:10)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
@codebytere
Copy link
Member Author

@codebytere codebytere commented Apr 14, 2020

hmm @targos - it pulls information from your local ncu configuration 🤔 i have the normal setup for myself and it finds that i'm codebytere successfully.

In re. the second issue - looks like cli doesn't handle nested spinners very well, i'll refactor.

And re. secure-tag - i added it to deps but we might need to simply require that a releaser has globally installed it instead and then execute it that way. What do you think?

lib/promote_release.js Outdated Show resolved Hide resolved
lib/promote_release.js Outdated Show resolved Hide resolved
lib/promote_release.js Outdated Show resolved Hide resolved
@targos
Copy link
Member

@targos targos commented Apr 14, 2020

@codebytere yeah, I actually did not have my .ncurc (it doesn't work well with the npm-check-updates lib and I have to move the file when I use it). I put it back in my home directory and it worked

@targos
Copy link
Member

@targos targos commented Apr 14, 2020

I wonder why it said that the PR does not have sufficient approvals. It has 6 of them 🤔

@targos
Copy link
Member

@targos targos commented Apr 14, 2020

This is what is passed to runSync for the secure tag:

/home/mzasso/git/nodejs/node-core-utils/node_modules/.bin/git-secure-tag [
  'v13.13.0',
  '813052119e9b73411534e2c50b027781e8882e10\n',
  '-sm',
  `"'2020-04-14, Node.js v13.13.0 (Current) Release"`
]
lib/promote_release.js Outdated Show resolved Hide resolved
lib/promote_release.js Outdated Show resolved Hide resolved
lib/promote_release.js Outdated Show resolved Hide resolved
@targos
Copy link
Member

@targos targos commented Apr 14, 2020

The branch switching doesn't work:

? Merge proposal branch into staging branch? Yes
⠋ Merging proposal branchError: Switched to branch 'v13.x'

    at exports.runSync (/home/mzasso/git/nodejs/node-core-utils/lib/run.js:60:11)
    at ReleasePromotion.mergeProposalBranch (/home/mzasso/git/nodejs/node-core-utils/lib/promote_release.js:292:5)
    at ReleasePromotion.promote (/home/mzasso/git/nodejs/node-core-utils/lib/promote_release.js:101:16)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
@targos
Copy link
Member

@targos targos commented Apr 14, 2020

^

That's because runSync throws child.stderr as an error if it's not empty. But git sometimes outputs its messages to stderr.
What should we do here?

if (!didResolveConflicts) {
cli.warn(`Aborting release promotion for version ${version}`);
return;
}

This comment has been minimized.

@targos

targos Apr 14, 2020
Member

else, we need to:

git add src/node_version.h # or ask the user to add conflicted files manually
git cherry-pick --continue
git push upstream master
git checkout branch-staging
lib/promote_release.js Outdated Show resolved Hide resolved
@targos
Copy link
Member

@targos targos commented Apr 14, 2020

The promotion step doesn't work. It stops immediately with a success message, but the promotion did not happen (note that the promotion script is interactive so we may have to run it in a special way)

@codebytere
Copy link
Member Author

@codebytere codebytere commented May 4, 2020

@targos this should be in a bit better shape now - i may also swap some more things to run asynchronously and need to figure out a better way to handle #402 (comment) 🤔 do you or perhaps @joyeecheung have some thoughts on best tactic to handle the stderr issue?

@codebytere codebytere force-pushed the codebytere:git-node-release-promote branch from fd50b34 to 42a9d9f May 4, 2020
@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented May 4, 2020

hm, I don't exactly remember why runSync throws on non-empty stderr, but I think we can switch the condition to if (child.status !== 0). If anything comes back weird we could switch it back and add an option like options.expectEmptyStderr for that particular use case.

@codebytere codebytere force-pushed the codebytere:git-node-release-promote branch from f6eee6a to 946d16d May 5, 2020
lib/promote_release.js Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the codebytere:git-node-release-promote branch from 946d16d to d944859 May 5, 2020
codebytere and others added 10 commits Apr 9, 2020
Co-Authored-By: Michaël Zasso <targos@protonmail.com>
@codebytere codebytere force-pushed the codebytere:git-node-release-promote branch from d944859 to c1ab158 May 20, 2020
@codebytere
Copy link
Member Author

@codebytere codebytere commented May 20, 2020

@targos rebased and updated a few things if you're willing to give this another spin on your next release!

@targos
Copy link
Member

@targos targos commented May 26, 2020

I'm giving it a try now with 12.17.0

lib/promote_release.js Outdated Show resolved Hide resolved
lib/promote_release.js Outdated Show resolved Hide resolved
codebytere added 2 commits May 26, 2020
@codebytere codebytere force-pushed the codebytere:git-node-release-promote branch from 0702242 to 3377b44 May 26, 2020
} else {
if (!releasers.some(r => r.login === release.username)) {
cli.stopSpinner(
`${release.username} is not a Releaser; aborting release`);
return;
}
cli.stopSpinner('Verified Releaser status');
}
Comment on lines +118 to +125

This comment has been minimized.

@lundibundi

lundibundi Aug 23, 2020
Member

Nit:

Suggested change
} else {
if (!releasers.some(r => r.login === release.username)) {
cli.stopSpinner(
`${release.username} is not a Releaser; aborting release`);
return;
}
cli.stopSpinner('Verified Releaser status');
}
} else if (releasers.every(r => r.login !== release.username)) {
cli.stopSpinner(
`${release.username} is not a Releaser; aborting release`);
return;
}
cli.stopSpinner('Verified Releaser status');
@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Nov 22, 2020

This PR is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.