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

feat: output checksums to artifacts info #3548

Merged
merged 6 commits into from Nov 14, 2022
Merged

Conversation

gal-legit
Copy link
Contributor

Following #3540, output artifacts' checksums to the artifact info.
This addition makes it easier to consume the checksums, especially when running from e.g. GitHub Actions.

New tests:

  1. Add a check for the checksum in the extra field.
  2. Add a test for every checksum algorithm (to see that it doesn't break for any algo's output).
  3. Add a case of a binary and an extra file (to see that the logic doesn't break when there's a mix).

p.s.
While working on that, I noticed that the convention for extra fields is actually to use UpperCamelCase rather than lower.
I was mistaken because I looked at the subfields of the "DockerConfig" extra field.
I think it's a good idea to fix it quickly, before the next release rolls and it becomes a compatibility issue.
I took the liberty to fix it here as an extra commit. Please let me know if you want it to be in a separate PR.


Tests:

go test
  • refreshing checksums                             file=binary_bar_checksums.txt
  • refreshing checksums                             file=binary_bar_checksums.txt
  • refreshing checksums                             file=binary_bar_checksums.txt
PASS
ok  	github.com/goreleaser/goreleaser/internal/pipe/checksums	0.184s

@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 13, 2022
@codecov
Copy link

codecov bot commented Nov 13, 2022

Codecov Report

Merging #3548 (864ff4a) into main (e1ab104) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Current head 864ff4a differs from pull request most recent head 3cd3007. Consider uploading reports for the commit 3cd3007 to get more accurate results

@@           Coverage Diff           @@
##             main    #3548   +/-   ##
=======================================
  Coverage   83.64%   83.65%           
=======================================
  Files         116      116           
  Lines        9692     9697    +5     
=======================================
+ Hits         8107     8112    +5     
  Misses       1284     1284           
  Partials      301      301           
Impacted Files Coverage Δ
internal/pipe/docker/docker.go 91.32% <ø> (ø)
internal/pipe/checksums/checksums.go 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

if a.Extra == nil {
a.Extra = make(artifact.Extras)
}
a.Extra[artifactChecksumExtra] = fmt.Sprintf("%s:%s", ctx.Config.Checksum.Algorithm, sum)
Copy link
Member

Choose a reason for hiding this comment

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

can we put this in the checksum func instead? I think we then don't need most of the rest of the code and it should simplify things quite a bit...

Copy link
Contributor Author

@gal-legit gal-legit Nov 14, 2022

Choose a reason for hiding this comment

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

That makes total sense. Not sure why I took the visit() approach instead of directly editing there in the first place.
done (also applied fixes from the lint)

@gal-legit
Copy link
Contributor Author

p.s. do you have a plan for when the next release (tag) is gonna be? we actually want to use these new fields for https://github.com/Legit-Labs/legitify

@caarlos0
Copy link
Member

@gal-legit probably late this week/weekend

Copy link
Member

@caarlos0 caarlos0 left a comment

Choose a reason for hiding this comment

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

I think we can revert the changes made to tests as the algos are already tested elsewhere...

what I think is missing is assering that the Extra["Checksum"] is not empty, other than that LGTM

"crc32": "f94d3859",
"md5": "5ac749fbeec93607fc28d666be85e73a",
"sha1": "8b45e4bd1c6acb88bebf6407d16205f567e62a3e",
"sha224": "21bc225587d8768058837b68fe7e0341e87b972f02fd8fb0c236d1d3",
"sha256": "61d034473102d7dac305902770471fd50f4c5b26f6831a56dd90b5184b3c30fc",
"sha384": "f6055a96a105d2fb5941a616964ffda8294fd415730cc4154a602062bc3d00e99d3c6f4a11af8c965a343de4afca3c2b",
"sha512": "14925e01a7a0cf0801aa95fe52d542b578af58ae7997ada66db3a6eae68a329d50600a5b7b442eabf4ea77ea8ef5fe40acf2ab31d47311b2a232c4f64009aac1",
}
Copy link
Member

Choose a reason for hiding this comment

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

this is already tested in artifacts_test.go, I think we can revert to before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added these tests to make sure that the extraction from checksums.txt is correct,
i.e. in case it'll be e.g. artifact-name sha-val instead of sha-val artifact-name,
and tbh I'm not sure it's covered in the other tests.

anyway, added a commit that does just that. I hope that I understood correctly and that it's the correct place to check it.

@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 14, 2022
@caarlos0
Copy link
Member

looks awesome, thank you!

@caarlos0 caarlos0 merged commit 76dc0b5 into goreleaser:main Nov 14, 2022
9 of 10 checks passed
@github-actions github-actions bot added this to the v1.13.0 milestone Nov 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants