c8d/list: Fix Total size calculation #48330
Conversation
vvoland
added
area/images
| // readConfig reads content pointed by the descriptor and unmarshals it into a specified output. | ||
| func readConfig(ctx context.Context, store content.Provider, desc ocispec.Descriptor, out interface{}) error { | ||
| // readJSON reads content pointed by the descriptor and unmarshals it into a specified output. | ||
| func readJSON(ctx context.Context, store content.Provider, desc ocispec.Descriptor, out interface{}) error { |
There was a problem hiding this comment.
Perhaps it could be extracted to the github.com/containerd/containerd/content package.
There's already content.ReadBlob , so I guess it could be named content.ReadJSONBlob.
There was a problem hiding this comment.
Ah, except our version also translates the cerrdefs.NotFound into errdefs.NotFound
There was a problem hiding this comment.
Ah, except our version also translates the cerrdefs.NotFound into errdefs.NotFound
That problem should probably go away at some point; at least, Derek is working on unifying errdefs to make them interoperable.
Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
vvoland
force-pushed
the
c8d-list-multiplatform-fixsize
branch
from
578ec00 to
6bb6bef
Compare
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
left one question, but good to go if that's not a concern
|
|
||
| // contentSize was already added to total, adjust it by the difference | ||
| // between the newly calculated size and the old size. | ||
| d := imgContentSize - contentSize |
There was a problem hiding this comment.
Any risk of contentSize being larger than imgContentSize here (so getting negative values?)
There was a problem hiding this comment.
Possibly, and that's also fine here - if imgContentSize is larger then adding a negative will still adjust the total size correctly.
There was a problem hiding this comment.
I had the exact same thought, and then realized it was fine either way 😅
There was a problem hiding this comment.
Ha! Yes, it just crossed my mind, so thought I'd double check. Thanks both!
| // readConfig reads content pointed by the descriptor and unmarshals it into a specified output. | ||
| func readConfig(ctx context.Context, store content.Provider, desc ocispec.Descriptor, out interface{}) error { | ||
| // readJSON reads content pointed by the descriptor and unmarshals it into a specified output. | ||
| func readJSON(ctx context.Context, store content.Provider, desc ocispec.Descriptor, out interface{}) error { |
There was a problem hiding this comment.
Ah, except our version also translates the cerrdefs.NotFound into errdefs.NotFound
That problem should probably go away at some point; at least, Derek is working on unifying errdefs to make them interoperable.
3 participants
- What I did
Fixed a bug where individual image sub-manifests were also accumulating the total size of the image.
- How I did it
- How to verify it
TestImageListCheckTotalSize
- Description for the changelog
No changelog, as the bug hasn't been released yet
- A picture of a cute animal (not mandatory but encouraged)