cmd, pkg/podman: Introduce Image Interface and Unit Tests#1724
cmd, pkg/podman: Introduce Image Interface and Unit Tests#1724DaliborKr wants to merge 2 commits intocontainers:mainfrom
Conversation
In reaction to the conversation in PR containers#1707, I made suggested changes to the implementation in terms of how Image information, received from Podman, is represented and treated in the Toolbx code. I used the same procedure as in the case of representing Containers: - Commit e611969 - Commit ec7eb59 In short, the JSON from 'podman inspect --type image' and 'podman images' are different, so logically, there should be different implementations of the JSON.Unmarshaler interface [1] for them as well. The new Image interface provides access to the values parsed from the JSONs and two different concrete types, which are implemented separately to handle the differences in the JSONs. GetImages() now returns an Images iterator-like structure that handles image flattening and sorting internally. InspectImage() returns an Image interface instead of map[string]interface{}, and the IsToolboxImage() function was replaced with the IsToolbx() method on the Image interface. [1] https://pkg.go.dev/encoding/json#Unmarshaler Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
Provides Go Unit tests for the Image interface implementation introduced in commit fd715a9. Tested were results of Podman starting on version 1.1.2, which should be sufficient since the minimum required Podman version is 1.6.4 (see commit 8e80dd5). Covered are structures representing podman commands 'podman inspect --type image' and 'podman images'. Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 06s |
debarshiray
left a comment
debarshiray
left a comment
There was a problem hiding this comment.
Thanks for working on this, @DaliborKr ! My apologies for the delay.
I am still reading through the changes, but one thing popped out at me.
| } | ||
|
|
||
| // ALREADY EXISTS IN CONTAINER.GO | ||
| // |
There was a problem hiding this comment.
I guess we need to take a decision about the code here in comments? :)
There was a problem hiding this comment.
I think we can move the code common to containers.go and images.go to podman.go.
| // | ||
| // If a problem happens during execution, first argument is nil and second argument holds the error message. | ||
| func GetImages(args ...string) ([]Image, error) { | ||
| func GetImages(fillNameWithID bool, sortByName bool, args ...string) (*Images, error) { |
There was a problem hiding this comment.
Isn't sortByName always set to true or did you have some future use in mind? Or did I miss something? :)
|
|
||
| func (images *Images) Get() Image { | ||
| if images.i < 1 { | ||
| panic("called Containers.Get() without calling Containers.Next()") |
There was a problem hiding this comment.
Typo alert: should be Images, not Containers. :)
| } | ||
|
|
||
| func (images Images) Len() int { | ||
| return len(images.data) |
There was a problem hiding this comment.
It might be better to change these pre-existing methods from ImageSlice that earlier had value receivers to pointer receivers.
I am not an expert on value versus pointer receivers, and I get confused all the time. It seems that the general advice is to not mix pointer and value receivers for a given type.
In the case of the Images type, the Reset() method must have a pointer receiver because it modifies a field of the type.
In case you wonder how the Swap() method was working, just like I did, it's because in Go slices are references to an underlying array and changing an element of a slice modifies the array and other slices see the same change. The value receiver copies the slice and the underlying array stays the same.
| } | ||
|
|
||
| sort.Sort(podman.ImageSlice(toolboxImages)) | ||
| return toolboxImages, nil |
There was a problem hiding this comment.
It looks like you are trying to get rid of this wrapper getImages() function, which is a good point. It also applies to the getContainers() function.
Other than the changes you have already made, I wonder if we need the wrappers to filter Toolbx containers and images. Do we ever use non-Toolbx containers and images?
As far as I can see, the wrappers originated when we were using podman images --filter and podman ps --filter to get the images instead of only podman images and podman ps.
I wonder if we can get rid of the wrappers completely, and if we can do it separately from this commit to keep the changes small.
2 participants
Introduces an Image interface based on the conversation in PR #1707.
Similar to how container information is handled (introduced in commits e611969 and ec7eb59), this PR abstracts the representation of an image. This is necessary because
podman inspect --type imageandpodman imagesreturn different JSON structures. The Image interface provides a unified way to access image data, with two distinct implementations to handle the different JSON formats.Updates related functions:
Adds unit tests: This PR includes unit tests for the new Image interface implementation, covering the output of both
podman inspect --type imageandpodman images.