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 #14363

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 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/build#513

Checklist
Affected core subsystem(s)

tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Jul 19, 2017
@gireeshpunathil gireeshpunathil added the build Issues and PRs related to build files or the CI. label Jul 19, 2017
@gireeshpunathil
Copy link
Member

@gdams - thanks. Is it platform neutral or specific to Linux?

@gdams
Copy link
Member Author

gdams commented Jul 19, 2017

So this is platform neutral, It essentially just downloads all of the tarballs/zips etc and runs a shasum command to ensure that the shasum is correctly documented in the SHASUMS256.txt file. The idea is that we will create a jenkins job that will download test each release

@mhdawson
Copy link
Member

This looks good, the main question I'd have is if it should be in the node repo. I'm thinking it likely belongs in the build repo instead. I'd suggest that it go in the tools directory in the build repo.

@gdams
Copy link
Member Author

gdams commented Jul 19, 2017

I'm happy to move this, I was just following @gibfahn's advice

echo "Error - SHASUMS256 does not match for ${file}"
echo "Expected - ${sha}"
echo "Found - ${remoteSHA}"
else
Copy link
Member

Choose a reason for hiding this comment

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

I think we'd want to have the script return non-zero when an error occurs

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/build#513
@gdams
Copy link
Member Author

gdams commented Jul 19, 2017

@mhdawson updated PTAL

@mhdawson
Copy link
Member

Unless anybody else thinks this should be part of the node repo, I'd we close and submit PR to build repo. At that point I can add a job to the CI that runs it on a regular basis.

@gdams
Copy link
Member Author

gdams commented Jul 19, 2017

moved to the build repo nodejs/build#804

@gibfahn
Copy link
Member

gibfahn commented Jul 23, 2017

@mhdawson I feel pretty strongly that this should be in nodejs/node.

  • This is a release script not a build script. It'll be managed by nodejs/release in the future.
  • We don't currently have any scripts in nodejs/build that aren't infra related
  • All the release scripts live in tools/
  • In future I expect this will be part of a release test setup.
  • I'd like to keep code related to node in nodejs/node unless there's a reason not to. Very few people know that the build repo exists, let alone what's in it.

@gibfahn
Copy link
Member

gibfahn commented Jul 23, 2017

I'd suggest that it go in the tools directory in the build repo.

That folder just contains jenkins-status.js, which seems to be a script to monitor Jenkins (i.e. infra related).

What would the reason for having it in nodejs/build be?

@mhdawson
Copy link
Member

mhdawson commented Jul 26, 2017

@gibfahn I suggested the build repo as something that will support a CI job. In many cases today we just have the logic in the CI jobs themselves, but I think it would be better to have that externalized to scripts on the build repo for many of the cases. The tools directories seemed as good as anything else.

I don't think we'd necessarily want it to be run as part of the regular release target as it is specifically bound to our download site. I can also see issues with it being bound to the node repo. For example if its part of a node release, we then re-vamp how donwloads are laid out then you might be faced with checking out 2 different versions of node.js, the one you want to build and then the one you need to do the download testing.

I could easily see it going into the hopefully soon to exist "nodejs/release" repo as opposed to the build repo as a tool that is managed/supported by the release team.

@gibfahn
Copy link
Member

gibfahn commented Jul 26, 2017

I suggested the build repo as something that will support a CI job. In many cases today we just have the logic in the CI jobs themselves, but I think it would be better to have that externalized to scripts on the build repo for many of the cases.

Why not just put it in nodejs/node? If we plan to move our scripts we probably want to use Jenkins pipelines (they seem to be the recommended way of doing things), and it'd be easiest just to keep it all in the one repo.

The tools directories seemed as good as anything else.

If we were going to start moving everything to build we should probably create a new directory specifically for Jenkins scripts.

I don't think we'd necessarily want it to be run as part of the regular release target as it is specifically bound to our download site.

I don't understand, the whole point is to verify that the releases are correctly uploaded to the download server right? I'd see this as being a sanity check before making releases public. If we could separate the upload process from the promotion process, then we could run this with other tests between upload and promotion/release.

For example if its part of a node release, we then re-vamp how downloads are laid out then you might be faced with checking out 2 different versions of node.js, the one you want to build and then the one you need to do the download testing.

True, but could we ever do that? I assume there'd be too much existing tooling in the wild for automating downloads for that to be possible. Also if you're doing a release then you're updating the branch anyway, so it's simple enough to just directly backport any changes to staging.


My experience is that things outside of nodejs/node don't get noticed and don't get reviewed. The current gap between the amount of review that Makefile changes get vs. CI script changes is huge, and I don't think moving scripts to nodejs/build or nodejs/release will help. I'd rather we start in nodejs/node, and move things out if we run into problems.

@mhdawson
Copy link
Member

Re ->

I don't understand, the whole point is to verify that the releases are correctly uploaded to the download server right? I'd see this as being a sanity check before making releases public. If we could separate the upload process from the promotion process, then we could run this with other tests between upload and promotion/release.

If we are the only ones that use the target. I'm not sure that is the case, what's to stop outside teams/companies from using the release target to build their releases. They would not want to test our downloads, right ?

@nodejs/build any opinions on the best place for the script ? I still lean towards either the build or release repo as opposed to nodejs/node due to the link to our download site, but if I'm in the minority I'm ok with moving back.

@gdams gdams deleted the download_testing branch July 17, 2018 12:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants