Repository navigation
feat: cache SHA384 entries - #298
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extends the internal cache to support multiple hashing algorithms (SHA-256 and SHA3-384) and updates callers/tests to specify which algorithm to use.
Changes:
- Introduced
hashAlgowith SHA-256 and SHA3-384 support, plus hash construction vianewHash. - Updated cache APIs (
Create,Write,Open,Read) to take an algorithm parameter and adjusted archive fetch code accordingly. - Added a test verifying SHA3-384 entries are isolated from SHA-256 entries.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/cache/cache.go | Adds multi-algorithm hashing support, updates cache APIs, and expires entries across algorithm directories. |
| internal/cache/cache_test.go | Updates tests to pass the algorithm and adds SHA3-384 isolation coverage. |
| internal/archive/archive.go | Updates cache usage in archive fetch path to specify SHA-256 algorithm. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
Signed-off-by: Paul Mars <[email protected]>
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Underlying logic is good, thanks. Needs polishing in terms of terminology.
|
|
||
| const ( | ||
| SHA256 hashAlgo = "sha256" | ||
| SHA384 hashAlgo = "sha384" |
There was a problem hiding this comment.
Public values/functions/etc with private types is an anti pattern.
Also, this already had a name, right above: digestKind. Note how the entire public API below talks about a "digest", not a "hash". The hash is the internal Go types and values used to construct them locally. The overall PR needs to be fixed to make the terminology clear and less conflicting.
Note that if you want to change everything to be "hash", I'm fine with that too. We just cannot sit in the middle and use both terms with no clear lines. If you do that, please don't use "algo" as there are better terms that are naturally short.
There was a problem hiding this comment.
Public values/functions/etc with private types is an anti pattern.
The type is now exported.
Also, this already had a name, right above: digestKind. Note how the entire public API below talks about a "digest", not a "hash". The hash is the internal Go types and values used to construct them locally. The overall PR needs to be fixed to make the terminology clear and less conflicting.
Note that if you want to change everything to be "hash", I'm fine with that too. We just cannot sit in the middle and use both terms with no clear lines. If you do that, please don't use "algo" as there are better terms that are naturally short.
I reworked it to use the existing digestKind term. I wanted to avoid a refactoring of the package in this PR.
| err := os.MkdirAll(filepath.Join(c.Dir, digestKind), 0755) | ||
| h, err := newHash(algo) | ||
| if err != nil { | ||
| return &Writer{err: fmt.Errorf("internal error: %v", err)} |
There was a problem hiding this comment.
This function got a call with a piece of data that is coming from a place it has no idea, and then it also got an error that it has no idea about, because it has not inspected it, from a different function. How does it know it's an internal error?
There was a problem hiding this comment.
This function was bringing more confusion than anything and was a unnecessary level of indirection. I inlined it. This is now more obvious why this is an internal error.
| var file *os.File | ||
| if digest == "" { | ||
| file, err = os.CreateTemp(c.filePath(""), "tmp.*") | ||
| file, err = os.CreateTemp(filepath.Join(c.Dir, string(algo)), "tmp.*") |
There was a problem hiding this comment.
Is the pattern from the existing code not working?
There was a problem hiding this comment.
Good catch. The existing pattern still works. I fixed it.
Signed-off-by: Paul Mars <[email protected]>
niemeyer
left a comment
niemeyer
left a comment
There was a problem hiding this comment.
Looks nice now, thanks!
|
The Binstore only publishes SHA3-384 hashes for bins. This PR enables storing entries in the cache using
SHA3-384 hashes, so it can store bins. Consumers of the
cachepackage must now explicitly select thealgorithm to be used to create/write/read an entry.