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
Conversation
Codecov Report
@@ 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
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
internal/pipe/checksums/checksums.go
Outdated
if a.Extra == nil { | ||
a.Extra = make(artifact.Extras) | ||
} | ||
a.Extra[artifactChecksumExtra] = fmt.Sprintf("%s:%s", ctx.Config.Checksum.Algorithm, sum) |
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.
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...
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.
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)
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 |
@gal-legit probably late this week/weekend |
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.
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", | ||
} |
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 is already tested in artifacts_test.go
, I think we can revert to before.
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.
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.
looks awesome, thank you! |
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:
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: