zstd:chunked refactoring for early review#2503
Conversation
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
Podman test PR: containers/podman#23506 |
| func CandidateTemplateWithCompression(v2Options *blobinfocache.CandidateLocations2Options, digest digest.Digest, compressorName string) *CandidateTemplate { | ||
| if v2Options == nil { | ||
| return true, types.PreserveOriginal, nil // Anything goes. The (compressionOp, compressionAlgo) values are not used. | ||
| return &CandidateTemplate{ // Anything goes. The CompressionOperation, CompressionAlgorithm values are not used. |
There was a problem hiding this comment.
-
If
compression{Type,Algorithm}fields are not used, it does not make much sense to initialize them explicitly -- we can just omit those and rely on default values assigned by Go (those seems to benilfor algo andtypes.PreserveOriginalfor op). -
nit:
s/Compression/compression/.
351bb81 to
5bb9639
Compare
| Location: types.BICLocationReference{Opaque: ""}, | ||
| }, | ||
| LastSeen: time.Time{}, |
There was a problem hiding this comment.
nit: these initializations may be omitted.
There was a problem hiding this comment.
I prefer being explicit here — especially about the fact that Time is not set.
internal/blobinfocache/types.go
Outdated
| // digest just because some remote author claims so (e.g. because a manifest says so); | ||
| // RecordDigestCompressorData records data for the blob with the specified digest. | ||
| // WARNING: Only call this with LOCALLY VERIFIED data: | ||
| // - don’t record a compressor for a digest just because some remote author claims so |
There was a problem hiding this comment.
nit: if you want this to be rendered as a list item in godoc, it needs to be indented. If you don't, maybe do not make it look like a list.
There was a problem hiding this comment.
(This is going to be a two-item list with #2487 .)
Godoc doesn’t show internal symbols, and seems not to format documentation of interface methods at all.
Anyway, approximately 0% of current doc strings are currently formatted to benefit from “the new” syntax, and we have to start somewhere. Updated.
kolyshkin
left a comment
There was a problem hiding this comment.
LGTM overall, left a few minor nits (can be ignored).
…er match The rules expect us to set manifest editing updates. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... and add CandidateWithLocation and CandidateWithUnknownLocation , so that the BIC implementations only need to deal with one value instead of carrying around three; we will want to add one more, and encapsulating them all into a single template will make it transparent to the cache implementations. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... just because we now can, and to nudge all future caches to be designed around CandidateTemplateWithCompression. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We will add more logic to the default case, so sharing the CandidateCompressionMatchesReuseConditions call is not going to be as easy. Split the two code paths. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
We will want to record more than a single alghoritm name. For now, just introduce the structure and modify users, we'll add the new fields later. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
… blobs ... because we don't trust the TOC data, if any. This allows us to remove the zstd:chunked hack; we, at least, now record those blobs as zstd. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
5bb9639 to
babdac8
Compare
This is a subset of #2487 , ready for review; it doesn’t make that much sense separately, but I’m filing it early to allow earlier review / merging, and to decrease the size of the later review. See #2487 for more on how this is intended to be used.
This is:
BaseVariantNameinstead of hard-coding a zstd:chunked exception.@giuseppe PTAL.