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

tools: add download testing #804

Closed
wants to merge 1 commit into from
Closed

Conversation

gdams
Copy link
Member

@gdams gdams commented Jul 19, 2017

This script will take an environment variable (NODE_VERSION) and
download all of the files from rsync://unencrypted.nodejs.org/nodejs/release/v<NODE_VERSION>/
It will then loop through the SHASUMS256.txt file to check if the
SHA256SUMS match up. Initial issue here: #513

mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

@gdams
Copy link
Member Author

gdams commented Jul 19, 2017

updated PTAL

@mhdawson
Copy link
Member

Started building the CI job, noted a few issues in earlier comments.

What I think would also be useful is if it could take an option like "latest" which would figure out the latest for a give release line. That we won't have to update the job as new releases come out. Generally I'm thinking we'll only need to be testing the latest.

@mhdawson
Copy link
Member

still getting this

cat: nodejs.org/dist/v8.1.4/SHASUMS256.txt: No such file or directory
./build/tools/download-test.sh: line 49: ${TAP}: ambiguous redirect

@gdams
Copy link
Member Author

gdams commented Jul 19, 2017

@mhdawson should be fixed now if you want to re-run?

@gdams
Copy link
Member Author

gdams commented Jul 19, 2017

CI job here for reference: https://ci.nodejs.org/view/All/job/validate-downloads/

@gireeshpunathil
Copy link
Member

I think the robot.txt is not allowing recursive download. We may need to parse the index.html to get links individually and then download them.

bash-4.1$ wget -r -d  http://nodejs.org/dist/v8.1.4/
...
Deciding whether to enqueue "http://nodejs.org/dist/v8.1.4/node-v8.1.4-x86.msi".
Rejecting path dist/v8.1.4/node-v8.1.4-x86.msi because of rule “dist/”.
Not following http://nodejs.org/dist/v8.1.4/node-v8.1.4-x86.msi because robots.txt forbids it.
Decided NOT to load it.
bash-4.1$ cat nodejs.org/robots.txt 
User-Agent: *
Disallow: /dist/
Disallow: /docs/
Allow: /dist/latest/
Allow: /dist/latest/docs/api/
Allow: /api/
bash-4.1$ 

@gireeshpunathil
Copy link
Member

ok, the latest script line
wget -r --no-parent --execute="robots = off" http://nodejs.org/dist/v${NODE_VERSION}/
seem to have addressed the robot block.

rvagg

This comment was marked as off-topic.

@mhdawson
Copy link
Member

Ok, so seems to run but I am seeing this

./build/tools/download-test.sh: line 36: [: !=: unary operator expected

@gdams I'd also like to be able to be able to specify something like 8.latest and have it figure out the latest version.

@rvagg
Copy link
Member

rvagg commented Jul 21, 2017

Worth noting that unencrypted.nodejs.org is currently on a 15 minute cron to update its binaries from the main server, that may need to be taken into account if this tool is for automating something close to release. The 15 minute number is arbitrary and could be changed if needed fwiw, it's even possible that we could use a flag mechanism to trigger a sync when a release is promoted so it's near instant, although that's a bit more complicated obviously.

@gibfahn
Copy link
Member

gibfahn commented Jul 22, 2017

@gdams when I said tools in #513 (comment) I linked to the tools/ directory in nodejs/node. I don't think it makes sense to start keeping scripts in this repo. so could you move this PR over there?

gibfahn

This comment was marked as off-topic.

@gdams
Copy link
Member Author

gdams commented Jul 22, 2017

@gibfahn I was asked by @mhdawson to move to over to nodejs/build nodejs/node#14363

@gibfahn
Copy link
Member

gibfahn commented Jul 23, 2017

@gdams ahh, don't know how I missed that PR. I'll comment over there.

@mhdawson
Copy link
Member

Now that I'm back I'd like to move forward on this. @gdams have you had time to look at my suggestions for it being able to automatically run using the latest release ?

@gdams
Copy link
Member Author

gdams commented Aug 16, 2017

@mhdawson @gibfahn updated PTAL. You can now either pass in a full node version (v6.11.2) or just (v6) and it will fetch the latest 6 build. You can also export DOWNLOAD_LOCATION=nightly and fetch the latest nightly builds

mhdawson

This comment was marked as off-topic.

@mhdawson
Copy link
Member

mhdawson commented Aug 16, 2017

@rvagg @jbergstroem any input on whether you think this best fits in the build or core node repo ? My feeling is the build repo but I could be convinced as @gibfahn suggests that it go into the core node repo.

@rvagg
Copy link
Member

rvagg commented Aug 16, 2017

Seems like a reasonable case could be made either way. If it's in nodejs/node then others could run it independently to confirm.

@mhdawson can you explain how the CI job runs? I can't quite work it out. It runs on a schedule but does that mean it's grabbing the latest version every hour to confirm? Also, you may want to make it run on jenkins-workspace because those machines are perfect for this kind of work.

rvagg

This comment was marked as off-topic.

@rvagg
Copy link
Member

rvagg commented Aug 17, 2017

just saw the description in #513 about how the script works, fine by me.

I've changed restrict nodes from ubuntu1404-64 || benchmark to jenkins-workspace which is serviced by two packet.net boxes with big 'ol disk.

rvagg

This comment was marked as off-topic.

@mhdawson
Copy link
Member

@rvagg thanks for the change to where it runs, I was wondering what the best machines would be.

gibfahn

This comment was marked as off-topic.

@gibfahn
Copy link
Member

gibfahn commented Aug 17, 2017

Seems like a reasonable case could be made either way. If it's in nodejs/node then others could run it independently to confirm.

Yeah it's a tricky one. Would be good to get opinions from more people in @nodejs/build.

@joaocgreis
Copy link
Member

My opinion on this is that nodejs/node is not the place for this. When we checkout a commit from nodejs/node, everything there should relate to that exact version, and be depended on by something there.

The way I would have done this would be directly in a Jenkins job script. We have backups of the server, so it would be safe. However, it would not be public and easily reviewable, but that again links to the discussion about reviewing the jobs source. Since this is already done as a script, I don't see why not land it here.

@mhdawson
Copy link
Member

@gibfahn I thinking from your +1 on @joaocgreis' comment that you are now ok with landing this this repo ? If so I assume we just need @gdams to address your latest review comments and we'll be good to land ?

@gibfahn
Copy link
Member

gibfahn commented Aug 18, 2017

@joaocgreis

My opinion on this is that nodejs/node is not the place for this. When we checkout a commit from nodejs/node, everything there should relate to that exact version, and be depended on by something there.

I don't think this applies to a lot of things in nodejs/node:tools/.

The way I would have done this would be directly in a Jenkins job script. We have backups of the server, so it would be safe. However, it would not be public and easily reviewable, but that again links to the discussion about reviewing the jobs source.

Yeah I think putting things in a Jenkins script is the worst possible option (we can't review, there may be backups but in my experience they aren't as easy to version control as git, it's hidden by default).

@mhdawson

I'm -0 on it, yes. If we're going to do this I'd like us to decide where we're going to put these kinds of scripts, and maybe move more out of Jenkins configs and into this repo. IDK if tools/ is the best name for this folder in that case, but no strong opinions.

Given that we have a meeting on Tuesday, we could probably talk about this there (in particular the general question of where files like this should go). I'll add it to the agenda.

@joaocgreis
Copy link
Member

I don't think this applies to a lot of things in nodejs/node:tools/.

In my opinion, it should. But I have no strong feelings about this.

Let's discuss at the meeting.

@gibfahn
Copy link
Member

gibfahn commented Aug 22, 2017

@gdams consensus in the meeting (minutes: #845) was to create a nodejs/build:/jenkins/ folder to put this in, as we might want to move more scripts in here going forwards.

If you could fix up my suggestions and change the folder, this should be good to go.

@gdams
Copy link
Member Author

gdams commented Aug 23, 2017

@gibfahn updated PTAL

richardlau

This comment was marked as off-topic.

gibfahn

This comment was marked as off-topic.

This script will take an environment variable (NODE_VERSION) and
download all of the files from `http://nodejs.org/dist/v<NODE_VERSION>/`
It will then loop through the `SHASUMS256.txt` file to check if the
SHA256SUMS match up. Initial issue here:
nodejs#513
@gdams
Copy link
Member Author

gdams commented Aug 24, 2017

@gibfahn updated PTAL

gibfahn

This comment was marked as off-topic.

@gibfahn
Copy link
Member

gibfahn commented Aug 24, 2017

@nodejs/build any other issues? If not I'll land this later today.

@gibfahn gibfahn self-assigned this Aug 24, 2017
@gdams
Copy link
Member Author

gdams commented Aug 24, 2017

remember that the download testing job needs to be updated with the new path

@mhdawson
Copy link
Member

About to land and then fix up the testing job as its already broken since it uses the branch :)

mhdawson pushed a commit that referenced this pull request Aug 24, 2017
This script will take an environment variable (NODE_VERSION) and
download all of the files from `http://nodejs.org/dist/v<NODE_VERSION>/`
It will then loop through the `SHASUMS256.txt` file to check if the
SHA256SUMS match up. Initial issue here:
#513

PR-URL: #804
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@mhdawson
Copy link
Member

Landed as 365c785

Jobs fixed up. Still need to discuss email notifications but we can do that in the original issue as opposed to this PR so closing.

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.

7 participants