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

Generate report on expected vs actual assets prior to release promotion #1596

Closed
wants to merge 5 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Nov 23, 2018

One for @nodejs/releasers. Please review.

This comes out of the borked 8.13.0 where we didn't realise x86 linux hadn't been compiled and promoted until someone complained.

This script would go alongside the promotion scripts and be inserted after you select a version to promote. You'd then have an additional confirmation step after this is run and you've seen the output. This script wouldn't force a halt to promotion, it's informational and the releaser is in charge of interpreting the output and taking appropriate action.

This is only the first step, I'll need to wire it in to _promote.sh if we're happy to go forward with this.

Here's the various cases that I've set it up to look for:

Everything is in staging as expected, good to go:
screenshot 2018-11-24 01 08 36

Some expected assets are not in staging:
screenshot 2018-11-24 01 09 14

Some assets have already been promoted that will be overwritten by a promotion of what's now in staging:
screenshot 2018-11-24 01 09 58

Everything that's already been promoted will be overwritten by what's in staging (a variation of the above case, but too many items to list)
screenshot 2018-11-24 01 10 53

We're trying to promote on a release line that we don't have a list of expected assets for (e.g. 12.0.0 and we've neglected to put in an expected list, understandable). In this case it will walk back up to 2 release lines and use the first one it finds instead:
screenshot 2018-11-24 01 12 26

Some assets in staging will complete the set that we already have promoted (e.g. armv6l files for 8.13.0):
screenshot 2018-11-24 01 14 22

Some unexpected assets are in staging:
screenshot 2018-11-24 01 15 08


So this script is stand-alone, tests are inside and it doesn't use any dependencies so it's uncomplicated to get it deployed. Node 10 is already on the www server so it's just a matter of executing it. It'll be put in the promote tool directory along with the files in ./expected_assets/. Each new release line will require a new file in that directory—that can go in the docs somewhere, probably nodejs/node/doc/releases.md, but as you can see above, it's also self-documenting about that.

I can imagine the expected assets files would be helpful for other purposes (like checking that we have everything across all historical versions) and I considered putting them elsewhere in the Build repo but couldn't find a logical place so kept it simple. I'm open to suggestions.

@refack
Copy link
Contributor

refack commented Nov 23, 2018

BTW, we have a Jenkins job validate-downloads that I think we asked @nodejs/releasers to run after a promotion, and IMO this script would fit in perfectly.

Refs: #804
Fixes: #83
Refs: #849
Refs: #940

@richardlau
Copy link
Member

Should the expected assets include the SHASUMS256.* files? (Working backwards from, e.g. https://nodejs.org/dist/latest-v10.x/ -- I'm less familiar with the release/promotion process).

@rvagg
Copy link
Member Author

rvagg commented Nov 24, 2018

@richardlau no, I don't think so since they are generated as a mandatory part of the promotion process and should never exist in staging.

But I've just pushed an update that properly accounts for them in dist, they shouldn't be considered extraneous if they show up there. I've hardwired them in there but I can imagine a future where we ditch SHASUMS256.txt.asc so maybe some special per-line logic will be called for then. Until then, the 3 files should exist in dist for all active release lines.

In the process I also added proper extraneous checking for dist:

screenshot 2018-11-24 10 58 29

SHASUMS files already in dist, but ignored:

screenshot 2018-11-24 10 57 31

@rvagg
Copy link
Member Author

rvagg commented Nov 27, 2018

Taking the chance with these security releases to test it out:

screenshot 2018-11-27 17 05 49

@rvagg
Copy link
Member Author

rvagg commented Nov 27, 2018

I didn't anticipate this but it's super helpful in the case where I have 4 release lines to do and ci-release isn't being cooperative and I'm having to rebuild individual assets because of build failures (mainly the Pi builds for older versions). So I have a heap of ci-release jobs running in parallel, all doing different things and trying to dump the right assets into staging for me, but I now can't keep track. But this is really nice for giving me confidence that I've got the right things ready to go:

screenshot 2018-11-27 17 16 36

@rvagg
Copy link
Member Author

rvagg commented Nov 27, 2018

I integrated it into the promote script and pushed that on the latest commit here. I ran a dry-run by commenting out the actual promotion part in promote_release.sh so I could get the whole flow without touching files.

v6.14.0 still has armv6l files missing so it's a great one to test live.

screenshot 2018-11-27 20 14 03

The bit at the bottom that says "promote nodejs v6.14.0" and "latest linker" is the stub for promotion, that won't be shown normally. Signing also failed because this release hasn't been tagged yet (that check should probably go before promotion but that's a separate task).

I also added nodejs/node#24669 to support the y/n option at the end. If you press n then it should just bail completely but currently it'll still try and sign something.

I'll test these releases live with this new stuff, after that I think it should be good to go for everyone else.

@@ -37,6 +37,12 @@ dirmatch=$test_dirmatch

. ${__dirname}/_promote.sh $site

srcdir=$v8_canary_srcdir
Copy link
Member Author

Choose a reason for hiding this comment

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

this shuffle is just matching the files here with what's on the server, there's only a difference in order of operations, nothing important

@rvagg
Copy link
Member Author

rvagg commented Nov 27, 2018

check tag before promote is nodejs/node#24670

@rvagg rvagg closed this Nov 30, 2018
@rvagg rvagg deleted the rvagg/check_assets branch November 30, 2018 02:02
Trott pushed a commit to Trott/io.js that referenced this pull request Dec 1, 2018
Ref: nodejs/build#1596

PR-URL: nodejs#24669
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BridgeAR pushed a commit to nodejs/node that referenced this pull request Dec 5, 2018
Ref: nodejs/build#1596

PR-URL: #24669
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
Ref: nodejs/build#1596

PR-URL: nodejs#24669
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit to nodejs/node that referenced this pull request Feb 12, 2019
Ref: nodejs/build#1596

PR-URL: #24669
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
rvagg added a commit to nodejs/node that referenced this pull request Feb 28, 2019
Ref: nodejs/build#1596

PR-URL: #24669
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants