c8d: Various images/json API fixes#46034
Conversation
| // byCreated is a temporary type used to sort a list of images by creation | ||
| // time. | ||
| type byCreated []*types.ImageSummary | ||
|
|
||
| func (r byCreated) Len() int { return len(r) } | ||
| func (r byCreated) Swap(i, j int) { r[i], r[j] = r[j], r[i] } | ||
| func (r byCreated) Less(i, j int) bool { return r[i].Created < r[j].Created } |
There was a problem hiding this comment.
Not a blocker, but starting to wonder if we need to move this somewhere central (I was planning to move the image types from types to types/image, so perhaps that's a good opportunity).
Also considering that sorting may at some point be something we want on the client side as well
| return nil, nil, err | ||
| } | ||
| var cfg configLabels | ||
| if err := readConfig(ctx, contentStore, cfgDesc, &cfg); err != nil { |
There was a problem hiding this comment.
LOL, very unrelated, but I was curious if we make use of the Data field anywhere in our code; https://github.com/opencontainers/image-spec/blob/v1.1.0-rc4/specs-go/v1/descriptor.go#L38-L41
// Data is an embedding of the targeted content. This is encoded as a base64
// string when marshalled to JSON (automatically, by encoding/json). If
// present, Data can be used directly to avoid fetching the targeted content.
Data []byte `json:"data,omitempty"`There was a problem hiding this comment.
I haven't seen any code using that yet TBH. Let me check, if it can be used that would be great actually
There was a problem hiding this comment.
It doesn't seem to be set by containerd, we might want to take a note of this and see if it can be used really
There was a problem hiding this comment.
Yup! I could see something along the lines of
if desc.Data != nil {
// fast path
}That said; I wonder if the Spec is clear enough on verifying Data matches the referenced data (would be fun if there's no such check and Data would return something very different from the referenced file 😂)
There was a problem hiding this comment.
There was a problem hiding this comment.
Nice, so yes, looks like we can start looking into using that
|
Some failures on Windows, which look related; |
b74fff4 to
5308d40
Compare
440a1f7 to
8c70f65
Compare
b0576bc to
bd86b37
Compare
vvoland
left a comment
There was a problem hiding this comment.
Overall LGTM, just have one performance nit.
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
left some nits (you know me 😂), but not blockers, so up to you if you want to address them ❤️
| return nil, nil, err | ||
| } | ||
| var cfg configLabels | ||
| if err := readConfig(ctx, contentStore, cfgDesc, &cfg); err != nil { |
There was a problem hiding this comment.
Nice, so yes, looks like we can start looking into using that
- use assert.Check to continue the test even if a check fails - assert the total number of images returned, not only their RepoTags - use subtests Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Aggregate same images into one object and add the list of tags pointing to it to the RepoTags array Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
Signed-off-by: Djordje Lukic <djordje.lukic@docker.com>
|
Aaaaaaaand.... all green |
- What I did
RepoTagswith all the tags pointing to an image instead of having multiple objects of the same image in the array.Fixes #43848
Fixes #43852
Also partially fix*s #43861, I didn't touch
RepoDigests- How I did it
...
- How to verify it
Run
make DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=TestAPIImagesFilter TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integrationmake DOCKER_GRAPHDRIVER=overlayfs TEST_FILTER=TestImagesFilterMultiReference TEST_INTEGRATION_USE_SNAPSHOTTER=1 test-integration- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)