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

hash: use generic instantiation #1538

Merged
merged 2 commits into from
Jan 24, 2023

Conversation

howardjohn
Copy link
Contributor

This enables doing something like
opencontainers/go-digest#71 (comment) to override the hashing method (example: replace with sha256simd).

When an importer does not overriden, the library should behave exactly the same.

See #1330

This enables doing something like
opencontainers/go-digest#71 (comment)
to override the hashing method (example: replace with sha256simd).

When an importer does not overriden, the library should behave exactly
the same.
@jonjohnsonjr
Copy link
Collaborator

Oh that's wonderful. A few more places:

registry/manifest.go
113:		rd := sha256.Sum256(m.blob)
140:		rd := sha256.Sum256(m.blob)
156:		rd := sha256.Sum256(b.Bytes())

registry/registry_test.go
50:	h := sha256.Sum256([]byte(s))
legacy/tarball/write.go
66:	rawDigest := sha256.Sum256([]byte(s))

@howardjohn
Copy link
Contributor Author

legacy/tarball/write.go

Thanks, missed those ones! Updated a bit. Note the ignored errors match what the old function did internally so I think its safe

@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2023

Codecov Report

Merging #1538 (62bb5a5) into main (efc62d8) will decrease coverage by 0.80%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1538      +/-   ##
==========================================
- Coverage   74.07%   73.27%   -0.80%     
==========================================
  Files         112      117       +5     
  Lines        8385     9015     +630     
==========================================
+ Hits         6211     6606     +395     
- Misses       1570     1748     +178     
- Partials      604      661      +57     
Impacted Files Coverage Δ
pkg/v1/stream/layer.go 88.05% <ø> (+0.38%) ⬆️
pkg/legacy/tarball/write.go 67.18% <100.00%> (ø)
pkg/registry/manifest.go 92.21% <100.00%> (-1.93%) ⬇️
pkg/v1/hash.go 89.09% <100.00%> (ø)
pkg/v1/random/image.go 78.57% <100.00%> (ø)
pkg/crane/append.go 55.17% <0.00%> (-15.67%) ⬇️
internal/retry/retry.go 86.20% <0.00%> (-13.80%) ⬇️
pkg/crane/options.go 79.16% <0.00%> (-6.55%) ⬇️
pkg/v1/remote/transport/useragent.go 41.17% <0.00%> (-5.89%) ⬇️
pkg/v1/remote/transport/transport.go 95.83% <0.00%> (-4.17%) ⬇️
... and 50 more

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

@jonjohnsonjr jonjohnsonjr merged commit d872232 into google:main Jan 24, 2023
@@ -47,8 +47,8 @@ const (
)

func sha256String(s string) string {
h := sha256.Sum256([]byte(s))
return hex.EncodeToString(h[:])
h, _, _ := v1.SHA256(bytes.NewReader([]byte(s)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

strings.NewReader could be better here

@imjasonh
Copy link
Collaborator

We might want a lint check that crypto/sha256 doesn't sneak back in in a future change, and to steer folks toward crypto.SHA256. I know I'm going to forget about this, old habits die hard.

jonjohnsonjr pushed a commit to jonjohnsonjr/go-containerregistry that referenced this pull request Jan 25, 2023
* hash: use generic instantiation

This enables doing something like
opencontainers/go-digest#71 (comment)
to override the hashing method (example: replace with sha256simd).

When an importer does not overriden, the library should behave exactly
the same.

* catch a few more usages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants