-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Validate emulator download by checking file size #1144
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
Conversation
bkendall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you need to run npm run format and you'll be good to go for the tests.
Why use the size rather than a hash? I can see either working okay, but hash would be slightly better in my opinion
|
src/emulator/download.js
Outdated
| let tmpFile = tmp.fileSync(); | ||
| let req = request.get(emulator.remoteUrl); | ||
| let writeStream = fs.createWriteStream(tmpFile.name); | ||
| let bytesDownloaded = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't look like this is used
|
|
Went ahead and added a checksum as well. Code is now a bit messy, would appreciate pointers here. |
bkendall
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good - I suggested a bit of refactoring around your else statements that should make it a bit nicer, but looks good from here!
src/emulator/download.js
Outdated
| { exit: 1 } | ||
| ) | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this else block can be removed if the reject above is returned
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and the same is true of the else block that this entire check is in)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
converted these to ternary statements, lmk if that's non-idiomatic
src/emulator/download.js
Outdated
| const hash = crypto.createHash('md5'), | ||
| const stream = fs.createReadStream(tmpFile.name); | ||
| stream.on('data', data => hash.update(data)); | ||
| stream.on('end', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be an arrow function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
src/emulator/download.js
Outdated
| stream.on('data', data => hash.update(data)); | ||
| stream.on('end', function() { | ||
| const checksum = hash.digest('hex'); | ||
| if (checksum != emulator.expectedHash) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (checksum != emulator.expectedHash) { | |
| if (checksum !== emulator.expectedHash) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#javascript
|
LGTM. fix up the CI and you're good to go. |
There are still customer reports of corrupted downloads that finish successfully. This is a fairly blunt way of double-checking that the download is valid.