go-get checksum file & guess checksum #136

azr · 2018-12-03T16:52:28Z

Hello,

This pr allows to checksum from a file; this should be the last needed feature for packer to be able to use go-getter 🙂

file_url?checksum=file:checksum_url

vancluever

Hey @azr, this is an interesting concept indeed, but I think we could possibly go about the hash detection in a much less complex way.

BSD formats notwithstanding, the hash should always be at the start of the file, and a hashed checksum is always going to be a static length depending on the algorithm:

MD5 is 16 bytes source
SHA1 is 20 bytes source
SHA256 is 32 bytes source
SHA512 is 64 bytes source

You could probably just take the sum, decode it from hex, and then check its size to get the right algorithm, and also pull the right file out accounting for specific the mode modifiers.

I've left some notes regarding recommending a refactoring of the functionality so that both query and file parsing go down two explicit code paths - that should make this easier to read.

Let me know when you've had a chance to update!

README.md

checksum_test.go

checksum.go

azr · 2018-12-13T10:33:27Z

Hey @vancluever !
Awesome review, thanks for reviewing ! Derp, I totally didn't think of using the lengths of the hashes. I was too focused tried to have pair functionality with packer 🙂.

checksum.go

azr · 2018-12-14T13:15:25Z

Okay, there, this should be done. Tell me what you think
I also added the possibility to checksum without passing the checksum type in the last commit. I totally can revert it if you think it's a bad idea. 🙂 I did it just because it was really simple to do from where I was and it makes usage simpler as you can now just pass checksum=<shasum>

README.md

vancluever

Hey @azr, looking a lot better, got some new comments to address, but fundamentally this is a great improvement!

source.go

test-fixtures/checksum-file/sha512-bsd.sum

vancluever · 2018-12-17T17:37:36Z

checksum.go

+//
+// checksumsFromFile will only return checksums for files that match file
+// behind src
+func checksumFromFile(checksumFile string, src *url.URL) (*fileChecksum, error) {


Hmm, I think I answered the question that I had from the docs here... we should really try and see how we can ensure that the same getter that is used to fetch the file is used to fetch the checksums. As mentioned, this is important on the HTTP getter where a custom client or additional headers will be used.

Okay, trying something 🙂 !

Just based on what I mentioned here, the client should probably just be passed in as a parameter to save having to apply several breaking changes to the code.

checksum.go

get.go

README.md

azr · 2018-12-20T13:58:52Z

Okay @vancluever I think this is better now, putting everything down methods/funcs had me cornered into a rabbit hole so instead I took a step back and put some code back/up in client.Get.

I also made a get func that is not a member of anything and takes all possible parameters. This allowed me to get the checksum file without breaking any api nor copying any code and it's not too bad !

This makes the body of get pretty long but I think this is for the best.

Tell me what you think 🙂

azr · 2019-01-08T15:12:43Z

Hello @vancluever, I updated this pull request to have the checksum create a copy of the client to download the checksum file. Like we decided.

Notes:
I am very glad I split commits by content as this was just a matter of reverting changing path and cherry-picking.

I also marked all convos as resolved for less clogging as this PR got huge. Thanks again for your patience.

checksum.go

client.go

common.go

vancluever · 2019-01-08T16:51:02Z

common.go

+	"io/ioutil"
+)
+
+func tmpFile(dir, pattern string) (string, error) {


I noticed that in testing references that there is already a tempFile helper (with the "e"). This is a bit confusing and I think that one should be used instead if it's not functionally different. If it's in a test helper file, just move it to a common helper file (ie: this common.go) so that it can be used in non-test code.

PS: If it needs to be extended to return errors, that's fine too. Just the point being that we should avoid having two tempfile helpers.

Okay, I simplified this... By adding a mustString func. I hope this is good 🙂

From what I can see, there's some other implementation details of tempFile that were missed. I'll have a look over it again and see if I can just manually correct it.

…better error

…_parseChecksumLine so we test everything from one entrypoint

TestGetFile_checksum_from_file tests it's cases we better

… its length

…e the hash at the beginning of line in an unexpected format

…parallel" This reverts commit f01d3d0.

they don't happen in the wild.

…les instead * also avoid protocol to be matched from the query parameters

azr · 2019-01-08T17:54:59Z

I just rebased this branch on master after #131 was merged 🙂

This reverts commit 7607f9c. The old test tempFile function is not general purpose and sets up a static file within a directory created by the tempDir helper. This functionality needs to be preserved, so the correct plan of action would be to preserve this functionality while moving the function name to another, logical name such as tempTestFile.

This renames the internal test tempFile function to tempTestFile. It also fixes some of the cleanup calls that were added - rather than deleting the static file created only, it deletes the directory, which is actually the specific temporary data created.

vancluever · 2019-01-08T19:12:16Z

Okay, as mentioned, the old test tempFile function needed fixing up still, because the re-implementation did not preserve the old functionality (returning a path for a static file "foo" within a directory created with tempDir).

This has now been corrected (you can see the changes in 6d796ed) and I'm just waiting for Travis to finish before merging.

azr mentioned this pull request Dec 3, 2018

go getter hashicorp/packer#6999

Merged

9 tasks

This comment has been minimized.

Sign in to view

azr changed the title ~~go-get checksum file & guess checksum~~ [WIP] go-get checksum file & guess checksum Dec 3, 2018

azr changed the title ~~[WIP] go-get checksum file & guess checksum~~ go-get checksum file & guess checksum Dec 4, 2018

vancluever suggested changes Dec 13, 2018

View reviewed changes

azr force-pushed the dl_checksum branch from 4a19b5d to 3680107 Compare December 13, 2018 10:23

azr commented Dec 14, 2018

View reviewed changes

checksum.go Outdated Show resolved Hide resolved

azr force-pushed the dl_checksum branch from 07415c7 to 064393a Compare December 14, 2018 11:55

vancluever reviewed Dec 17, 2018

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

vancluever suggested changes Dec 17, 2018

View reviewed changes

vancluever reviewed Dec 18, 2018

View reviewed changes

get.go Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

azr force-pushed the dl_checksum branch 4 times, most recently from 81c5385 to c795398 Compare December 20, 2018 13:55

azr force-pushed the dl_checksum branch from 24c32bb to 7eabbd5 Compare January 8, 2019 14:58

vancluever suggested changes Jan 8, 2019

View reviewed changes

azr added 9 commits January 8, 2019 18:53

allow to get checksum from go-gettere'd file

a67abad

checksum from file: error managment + tests + fixes

ee889a1

checksumsFromFile: name tmp file identically as checksum file

59bde3e

doc checksumsFromFile

5c32c13

add bad gnu checksum test for checksumsFromFile

d070edd

better error message in checksumHashAndValue

f5f3893

doc more

e47d598

checksumHashAndValue return a type even if there is no semicolor for …

dd14d29

…better error

add Test_checksumHashAndValue + test with local files and remove Test…

46d4103

…_parseChecksumLine so we test everything from one entrypoint

azr added 20 commits January 8, 2019 18:53

go-imports

4c4dfb6

add more tests in TestGetFile_checksum_from_file

6c797bb

Delete checksum_test.go

581bb3d

TestGetFile_checksum_from_file tests it's cases we better

remove tempFile in tests

0ca2927

allow to not pass the checksumming type in order to guess it based on…

a8aa720

… its length

file checksum: also allow to for file to only contain the hash or hav…

76aee45

…e the hash at the beginning of line in an unexpected format

Revert "make tempFile not clean the tempDir as tests could happen in …

5b3a85d

…parallel" This reverts commit f01d3d0.

checksum: add test for sha1 file only containing sha1

2a247a3

remove tests on bsd formated shas checksum files

f60bf8f

they don't happen in the wild.

update doc about checksum file get behavior

e4ac591

remove named parameters of SourceDirSubdir and document it with examp…

a6260be

…les instead * also avoid protocol to be matched from the query parameters

also test getting a checksum from a httptest Server

af65cf6

Update README.md

91417d4

Update doc for checksumsFromFile

707413a

be more precise in the README about temporary location selection

9c5e467

don't ignore path errors

8747bdc

checksumming: simplify a defer os.Remove(tempfile) call

666e5d0

move checksum func to checksum.go

82e7318

simplify tmpFile call

9693100

merge tmpFile and tempFile usages

7607f9c

azr force-pushed the dl_checksum branch from aed2b01 to 7607f9c Compare January 8, 2019 17:54

vancluever added 2 commits January 8, 2019 10:48

vancluever merged commit fd95e62 into master Jan 8, 2019

azr deleted the dl_checksum branch January 9, 2019 14:12

azr mentioned this pull request Jan 11, 2019

Enhancement: allow relative local file for checksums hashicorp/packer#5621

Closed

sagikazarmark mentioned this pull request Jan 15, 2019

Add checksum validation sagikazarmark/binbrew#32

Open

azr mentioned this pull request Feb 5, 2019

Various fixes #163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

go-get checksum file & guess checksum #136

go-get checksum file & guess checksum #136

azr commented Dec 3, 2018 •

edited

Loading

This comment has been minimized.

vancluever left a comment •

edited

Loading

azr commented Dec 13, 2018 •

edited

Loading

azr commented Dec 14, 2018 •

edited

Loading

vancluever left a comment

vancluever Dec 17, 2018

azr Dec 18, 2018

vancluever Dec 18, 2018

azr commented Dec 20, 2018 •

edited

Loading

azr commented Jan 8, 2019 •

edited

Loading

vancluever Jan 8, 2019

vancluever Jan 8, 2019

azr Jan 8, 2019 •

edited

Loading

vancluever Jan 8, 2019

azr commented Jan 8, 2019 •

edited

Loading

vancluever commented Jan 8, 2019

go-get checksum file & guess checksum #136

go-get checksum file & guess checksum #136

Conversation

azr commented Dec 3, 2018 • edited Loading

This comment has been minimized.

vancluever left a comment • edited Loading

Choose a reason for hiding this comment

azr commented Dec 13, 2018 • edited Loading

azr commented Dec 14, 2018 • edited Loading

vancluever left a comment

Choose a reason for hiding this comment

vancluever Dec 17, 2018

Choose a reason for hiding this comment

azr Dec 18, 2018

Choose a reason for hiding this comment

vancluever Dec 18, 2018

Choose a reason for hiding this comment

azr commented Dec 20, 2018 • edited Loading

azr commented Jan 8, 2019 • edited Loading

vancluever Jan 8, 2019

Choose a reason for hiding this comment

vancluever Jan 8, 2019

Choose a reason for hiding this comment

azr Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

vancluever Jan 8, 2019

Choose a reason for hiding this comment

azr commented Jan 8, 2019 • edited Loading

vancluever commented Jan 8, 2019

azr commented Dec 3, 2018 •

edited

Loading

vancluever left a comment •

edited

Loading

azr commented Dec 13, 2018 •

edited

Loading

azr commented Dec 14, 2018 •

edited

Loading

azr commented Dec 20, 2018 •

edited

Loading

azr commented Jan 8, 2019 •

edited

Loading

azr Jan 8, 2019 •

edited

Loading

azr commented Jan 8, 2019 •

edited

Loading