zstd:chunked refactoring for early review #2503
Conversation
Signed-off-by: Miloslav Trmač <[email protected]>
|
Podman test PR: containers/podman#23506 |
| 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/.
mtrmac
force-pushed
the
chunked-bic2-partial
branch
from
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.
…er match The rules expect us to set manifest editing updates. Signed-off-by: Miloslav Trmač <[email protected]>
... 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č <[email protected]>
... 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č <[email protected]>
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č <[email protected]>
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č <[email protected]>
… 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č <[email protected]>
mtrmac
force-pushed
the
chunked-bic2-partial
branch
from
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.