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.

Sign up for GitHub

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

Jump to bottom

Verify downloaded test runner zip file 812 #4193

Merged
merged 13 commits into from
Jul 9, 2019
Merged

Verify downloaded test runner zip file 812 #4193

merged 13 commits into from
Jul 9, 2019

Conversation

bahmutov
Copy link
Contributor

@bahmutov bahmutov commented May 14, 2019 •
edited
Loading

  • uses custom meta headers from CDN to verify the downloaded file. If the checksum is available, uses both checksum and file size. If only file size is available, which is content-length, uses file size. Otherwise no verification, but it is hard to think of the case when there is no information
  • closes Check downloaded binary file size #812
  • closes Unzipping Cypress fails on Concourse #3515
  • add a test where download goes through, but the checksum is different
  • add a test where download goes through, there is no checksum, but file size is different

@bahmutov bahmutov changed the title Verify download file 812 WIP: Verify download file 812 May 14, 2019
@bahmutov bahmutov changed the title WIP: Verify download file 812 Verify download file 812 May 14, 2019
@bahmutov bahmutov requested a review from kuceb May 14, 2019 20:41
@kuceb
Copy link
Contributor

kuceb commented May 14, 2019

@bahmutov
Could you point me to where you got those header names? I found this https://docs.aws.amazon.com/AmazonS3/latest/API/RESTCommonRequestHeaders.html

which says the hash header name is x-amz-content-sha256

@bahmutov
Copy link
Contributor Author

bahmutov commented May 14, 2019 via email

@kuceb
Copy link
Contributor

kuceb commented May 14, 2019 •
edited
Loading

oh, nice. And its a sha-512 ?

edit: maybe add a comment explaining where the header comes from

@bahmutov
Copy link
Contributor Author

bahmutov commented May 15, 2019 via email

jennifer-shehane
jennifer-shehane requested changes May 15, 2019
View reviewed changes
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment •
edited
Loading

Choose a reason for hiding this comment

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

All dependencies within the cli package should support Node 4+, hasha, after 3.0.0, does not support Node 4, I'd recommend downgrading this package to 3.0.0 or require a new package that supports Node 4.

@bahmutov
Copy link
Contributor Author

bahmutov commented May 15, 2019 via email

@jennifer-shehane
Copy link
Member

Yah, probably a discussion we need to have.

@kuceb
Copy link
Contributor

kuceb commented May 15, 2019

Hasha defaults to sha-512 according to readme https://github.com/sindresorhus/hasha/blob/master/readme.md#algorithm

@bahmutov
Copy link
Contributor Author

bahmutov commented May 15, 2019 via email

@bahmutov bahmutov changed the title Verify download file 812 Verify downloaded test runner zip file 812 May 15, 2019
@bahmutov bahmutov mentioned this pull request May 15, 2019
Closed
@bahmutov
Copy link
Contributor Author

I moved hasha to be dev dependency of CLI - and lifted the little code snippet for computing sha512 hash of a file using just system Node dependencies

kuceb
kuceb previously approved these changes May 15, 2019
View reviewed changes
@bahmutov
Copy link
Contributor Author

Since I removed Hasha as a production dependency in CLI, it is no longer changing Node 4 support

@bahmutov bahmutov dismissed stale reviews from jennifer-shehane and kuceb via 5efccab May 20, 2019 20:29
@bahmutov bahmutov requested a review from brian-mann May 20, 2019 21:13
@bahmutov
Copy link
Contributor Author

verifying the downloaded file should resolve unzipping issues reported in #3515

@jennifer-shehane jennifer-shehane self-requested a review May 21, 2019 04:24
@joekrump joekrump mentioned this pull request Jun 14, 2019
Closed
@bfagundez
Copy link

Hello, seems like the test failing is in fact the same problem we are having here.
Is this going to be looked into anytime soon?

@jennifer-shehane
Copy link
Member

@bahmutov This test is consistently failing on your PR, can you take a look please?

flotwig
flotwig previously approved these changes Jul 9, 2019
View reviewed changes
Copy link
Contributor

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

LGTM once failing test is fixed

cli/test/lib/tasks/download_spec.js
.reply(200, () => {
return fs.createReadStream('test/fixture/example.zip')
}, {
'x-amz-meta-checksum': 'incorrect-checksum',
Copy link
Contributor

Choose a reason for hiding this comment

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

neat, didn't know you could add headers to s3 resources like this

@flotwig flotwig merged commit c57d302 into develop Jul 9, 2019
@cypress-bot cypress-bot bot mentioned this pull request Jul 9, 2019
Closed
@emilyrohrbough emilyrohrbough deleted the verify-download-file-812 branch August 1, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietrofxq pietrofxq pietrofxq left review comments

@brian-mann brian-mann brian-mann left review comments

@flotwig flotwig flotwig approved these changes

@kuceb kuceb kuceb left review comments

@jennifer-shehane jennifer-shehane Awaiting requested review from jennifer-shehane

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unzipping Cypress fails on Concourse Check downloaded binary file size
7 participants
@bahmutov @kuceb @jennifer-shehane @bfagundez @flotwig @brian-mann @pietrofxq