cmd, pkg/podman: Turn Image into an interface#1779
cmd, pkg/podman: Turn Image into an interface#1779debarshiray merged 3 commits intocontainers:mainfrom
Conversation
Fallout from 6aab0a6 and e772207 containers#1779
debarshiray
force-pushed
the
wip/rishi/DaliborKr-image_interface-00
branch
from
5e70bd6 to
b611c8f
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors Podman image handling by introducing an Image interface and an Images iterator, replacing the previous slice-based implementation across the completion, list, and rmi commands. The changes include moving image-related logic to a new image.go file and adding extensive unit tests. Feedback suggests enhancing the nil-safety of the Images iterator methods and optimizing the Get method to return pointers directly from the underlying slice to avoid unnecessary struct copies.
I am having trouble creating individual review comments. Click here to see my feedback.
src/pkg/podman/image.go (46-53)
The Get() method currently creates an unnecessary copy of the imageImages struct before returning its address. Since imageImages implements the Image interface via pointer receivers, you can return a pointer to the element in the slice directly. This avoids the overhead of copying the struct and the heap allocation for the local variable. Additionally, for consistency with Len(), it's better to include a nil check for the receiver.
func (images *Images) Get() Image {
if images == nil {
panic("called Images.Get() on a nil Images pointer")
}
if images.i < 1 {
panic("called Images.Get() without calling Images.Next()")
}
return &images.data[images.i-1]
}src/pkg/podman/image.go (63-70)
The Next() method is not nil-safe, which is inconsistent with the Len() method at line 55. If images is nil, calling Next() will cause a panic. Adding a nil check ensures that the iterator behaves safely and consistently when used on a nil pointer (e.g., in a for images.Next() { ... } loop).
func (images *Images) Next() bool {
if images == nil {
return false
}
available := images.i < len(images.data)
if available {
images.i++
}
return available
}
src/pkg/podman/image.go (72-74)
The Reset() method should be made nil-safe for consistency with the Len() method. This prevents a panic if Reset() is called on a nil *Images pointer.
func (images *Images) Reset() {
if images == nil {
return
}
images.i = 0
}
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 10s |
A subsequent commit will switch to unmarshalling the JSON returned from 'podman inspect --format json --type image' directly inside podman.InspectImage() to confine the details within the podman package and make it easier to write unit tests for it. eg., it requires tracking changes to the JSON output across different Podman versions. Unfortunately, the JSON from 'podman inspect --format json --type image' and 'podman images --format json' are considerably different and it will be awkward to use the same implementation of the json.Unmarshaler interface [1] for both. One option is to have two different concrete types separately implement json.Unmarshaler to handle the differences in the JSON, and then hiding these concrete types behind an Image interface that provides access to the values parsed from the JSON. The JSON samples for the unit tests were taken using the default Toolbx image on versions of Fedora that shipped a specific Podman and Toolbx version. This accounts for differences in the JSON caused by different major versions of Podman and the way different Toolbx images were built. One exception was Fedora 28, which had Podman 1.1.2 and Toolbx 0.0.9, which was the last Toolbx version before 'toolbox init-container' became the entry point for all Toolbx containers [2]. However, the default Toolbx image is no longer available from registry.fedoraproject.org. Hence, the image for Fedora 29 was used. The minimum required Podman version is 1.6.4 [3], and the Go implementation has been encouraging users to create containers with Toolbx version 0.0.97 or newer [4]. The versions used to collect the JSON samples for the unit tests were chosen accordingly. They don't exhaustively cover all possible supported and unsupported version combinations, but hopefully enough to be useful. The fedora-toolbox image went through a series of significant changes during the Fedora 39 and 40 development cycles. Fedora 38 was the last release where the image was built from a Container/Dockerfile [5]. For Fedora 39, it was rewritten in terms of fedora-kickstarts and pungi-fedora for the ToolbxReleaseBlocker Change [6]. For Fedora 40, it was rewritten as a KIWI description as part of the KiwiBuiltCloudImages Change [7]. Hence, all three Fedora versions were chosen to cover this transition. [1] https://pkg.go.dev/encoding/json#Unmarshaler [2] Commit 8b84b5e containers@8b84b5e4604921fa containers#160 [3] Commit 8e80dd5 containers@8e80dd5db1e6f40b containers#1253 [4] Commit af1216b containers@af1216b2720c7ab5 containers#1697 containers#1684 [5] https://src.fedoraproject.org/container/fedora-toolbox [6] https://fedoraproject.org/wiki/Changes/ToolbxReleaseBlocker [7] https://fedoraproject.org/wiki/Changes/KiwiBuiltCloudImages containers#1724 containers#1779
A subsequent commit will switch to unmarshalling the JSON returned from 'podman inspect --format json --type image' directly inside podman.InspectImage() to confine the details within the podman package and make it easier to write unit tests for it. eg., it requires tracking changes to the JSON output across different Podman versions. Unfortunately, the JSON from 'podman inspect --format json --type image' and 'podman images --format json' are considerably different and it will be awkward to use the same implementation of the json.Unmarshaler interface [1] for both. One option is to have two different concrete types separately implement json.Unmarshaler to handle the differences in the JSON, and then hiding these concrete types behind an Image interface that provides access to the values parsed from the JSON. The JSON samples for the unit tests were taken using the default Toolbx image on versions of Fedora that shipped a specific Podman and Toolbx version. This accounts for differences in the JSON caused by different major versions of Podman and the way different Toolbx images were built. One exception was Fedora 28, which had Podman 1.1.2 and Toolbx 0.0.9, which was the last Toolbx version before 'toolbox init-container' became the entry point for all Toolbx containers [2]. However, the default Toolbx image is no longer available from registry.fedoraproject.org. Hence, the image for Fedora 29 was used. The minimum required Podman version is 1.6.4 [3], and the Go implementation has been encouraging users to create containers with Toolbx version 0.0.97 or newer [4]. The versions used to collect the JSON samples for the unit tests were chosen accordingly. They don't exhaustively cover all possible supported and unsupported version combinations, but hopefully enough to be useful. The fedora-toolbox image went through a series of significant changes during the Fedora 39 and 40 development cycles. Fedora 38 was the last release where the image was built from a Container/Dockerfile [5]. For Fedora 39, it was rewritten in terms of fedora-kickstarts and pungi-fedora for the ToolbxReleaseBlocker Change [6]. For Fedora 40, it was rewritten as a KIWI description as part of the KiwiBuiltCloudImages Change [7]. Hence, all three Fedora versions were chosen to cover this transition. [1] https://pkg.go.dev/encoding/json#Unmarshaler [2] Commit 8b84b5e containers@8b84b5e4604921fa containers#160 [3] Commit 8e80dd5 containers@8e80dd5db1e6f40b containers#1253 [4] Commit af1216b containers@af1216b2720c7ab5 containers#1697 containers#1684 [5] https://src.fedoraproject.org/container/fedora-toolbox [6] https://fedoraproject.org/wiki/Changes/ToolbxReleaseBlocker [7] https://fedoraproject.org/wiki/Changes/KiwiBuiltCloudImages containers#1724 containers#1779
debarshiray
force-pushed
the
wip/rishi/DaliborKr-image_interface-00
branch
from
ab763a9 to
044b8a7
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 05s |
Good point! Fixed. |
A subsequent commit will switch to unmarshalling the JSON returned from 'podman inspect --format json --type image' directly inside podman.InspectImage() to confine the details within the podman package and make it easier to write unit tests for it. eg., it requires tracking changes to the JSON output across different Podman versions. Unfortunately, the JSON from 'podman inspect --format json --type image' and 'podman images --format json' are considerably different and it will be awkward to use the same implementation of the json.Unmarshaler interface [1] for both. One option is to have two different concrete types separately implement json.Unmarshaler to handle the differences in the JSON, and then hiding these concrete types behind an Image interface that provides access to the values parsed from the JSON. The JSON samples for the unit tests were taken using the default Toolbx image on versions of Fedora that shipped a specific Podman and Toolbx version. This accounts for differences in the JSON caused by different major versions of Podman and the way different Toolbx images were built. One exception was Fedora 28, which had Podman 1.1.2 and Toolbx 0.0.9, which was the last Toolbx version before 'toolbox init-container' became the entry point for all Toolbx containers [2]. However, the default Toolbx image is no longer available from registry.fedoraproject.org. Hence, the image for Fedora 29 was used. The minimum required Podman version is 1.6.4 [3], and the Go implementation has been encouraging users to create containers with Toolbx version 0.0.97 or newer [4]. The versions used to collect the JSON samples for the unit tests were chosen accordingly. They don't exhaustively cover all possible supported and unsupported version combinations, but hopefully enough to be useful. The fedora-toolbox image went through a series of significant changes during the Fedora 39 and 40 development cycles. Fedora 38 was the last release where the image was built from a Container/Dockerfile [5]. For Fedora 39, it was rewritten in terms of fedora-kickstarts and pungi-fedora for the ToolbxReleaseBlocker Change [6]. For Fedora 40, it was rewritten as a KIWI description as part of the KiwiBuiltCloudImages Change [7]. Hence, all three Fedora versions were chosen to cover this transition. [1] https://pkg.go.dev/encoding/json#Unmarshaler [2] Commit 8b84b5e containers@8b84b5e4604921fa containers#160 [3] Commit 8e80dd5 containers@8e80dd5db1e6f40b containers#1253 [4] Commit af1216b containers@af1216b2720c7ab5 containers#1697 containers#1684 [5] https://src.fedoraproject.org/container/fedora-toolbox [6] https://fedoraproject.org/wiki/Changes/ToolbxReleaseBlocker [7] https://fedoraproject.org/wiki/Changes/KiwiBuiltCloudImages containers#1724 containers#1779
debarshiray
force-pushed
the
wip/rishi/DaliborKr-image_interface-00
branch
from
044b8a7 to
dde5f4b
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 07s |
Good point! Fixed. |
A subsequent commit will switch to unmarshalling the JSON returned from 'podman inspect --format json --type image' directly inside podman.InspectImage() to confine the details within the podman package and make it easier to write unit tests for it. eg., it requires tracking changes to the JSON output across different Podman versions. Unfortunately, the JSON from 'podman inspect --format json --type image' and 'podman images --format json' are considerably different and it will be awkward to use the same implementation of the json.Unmarshaler interface [1] for both. One option is to have two different concrete types separately implement json.Unmarshaler to handle the differences in the JSON, and then hiding these concrete types behind an Image interface that provides access to the values parsed from the JSON. The JSON samples for the unit tests were taken using the default Toolbx image on versions of Fedora that shipped a specific Podman and Toolbx version. This accounts for differences in the JSON caused by different major versions of Podman and the way different Toolbx images were built. One exception was Fedora 28, which had Podman 1.1.2 and Toolbx 0.0.9, which was the last Toolbx version before 'toolbox init-container' became the entry point for all Toolbx containers [2]. However, the default Toolbx image is no longer available from registry.fedoraproject.org. Hence, the image for Fedora 29 was used. The minimum required Podman version is 1.6.4 [3], and the Go implementation has been encouraging users to create containers with Toolbx version 0.0.97 or newer [4]. The versions used to collect the JSON samples for the unit tests were chosen accordingly. They don't exhaustively cover all possible supported and unsupported version combinations, but hopefully enough to be useful. The fedora-toolbox image went through a series of significant changes during the Fedora 39 and 40 development cycles. Fedora 38 was the last release where the image was built from a Container/Dockerfile [5]. For Fedora 39, it was rewritten in terms of fedora-kickstarts and pungi-fedora for the ToolbxReleaseBlocker Change [6]. For Fedora 40, it was rewritten as a KIWI description as part of the KiwiBuiltCloudImages Change [7]. Hence, all three Fedora versions were chosen to cover this transition. [1] https://pkg.go.dev/encoding/json#Unmarshaler [2] Commit 8b84b5e containers@8b84b5e4604921fa containers#160 [3] Commit 8e80dd5 containers@8e80dd5db1e6f40b containers#1253 [4] Commit af1216b containers@af1216b2720c7ab5 containers#1697 containers#1684 [5] https://src.fedoraproject.org/container/fedora-toolbox [6] https://fedoraproject.org/wiki/Changes/ToolbxReleaseBlocker [7] https://fedoraproject.org/wiki/Changes/KiwiBuiltCloudImages containers#1724 containers#1779
debarshiray
force-pushed
the
wip/rishi/DaliborKr-image_interface-00
branch
from
dde5f4b to
f858321
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 13s |
A subsequent commit will switch to unmarshalling the JSON returned from 'podman inspect --format json --type image' directly inside podman.InspectImage() to confine the details within the podman package and make it easier to write unit tests for it. eg., it requires tracking changes to the JSON output across different Podman versions. Unfortunately, the JSON from 'podman inspect --format json --type image' and 'podman images --format json' are considerably different and it will be awkward to use the same implementation of the json.Unmarshaler interface [1] for both. One option is to have two different concrete types separately implement json.Unmarshaler to handle the differences in the JSON, and then hiding these concrete types behind an Image interface that provides access to the values parsed from the JSON. The JSON samples for the unit tests were taken using the default Toolbx image on versions of Fedora that shipped a specific Podman and Toolbx version. This accounts for differences in the JSON caused by different major versions of Podman and the way different Toolbx images were built. One exception was Fedora 28, which had Podman 1.1.2 and Toolbx 0.0.9, which was the last Toolbx version before 'toolbox init-container' became the entry point for all Toolbx containers [2]. However, the default Toolbx image is no longer available from registry.fedoraproject.org. Hence, the image for Fedora 29 was used. The minimum required Podman version is 1.6.4 [3], and the Go implementation has been encouraging users to create containers with Toolbx version 0.0.97 or newer [4]. The versions used to collect the JSON samples for the unit tests were chosen accordingly. They don't exhaustively cover all possible supported and unsupported version combinations, but hopefully enough to be useful. The fedora-toolbox image went through a series of significant changes during the Fedora 39 and 40 development cycles. Fedora 38 was the last release where the image was built from a Container/Dockerfile [5]. For Fedora 39, it was rewritten in terms of fedora-kickstarts and pungi-fedora for the ToolbxReleaseBlocker Change [6]. For Fedora 40, it was rewritten as a KIWI description as part of the KiwiBuiltCloudImages Change [7]. Hence, all three Fedora versions were chosen to cover this transition. [1] https://pkg.go.dev/encoding/json#Unmarshaler [2] Commit 8b84b5e containers@8b84b5e4604921fa containers#160 [3] Commit 8e80dd5 containers@8e80dd5db1e6f40b containers#1253 [4] Commit af1216b containers@af1216b2720c7ab5 containers#1697 containers#1684 [5] https://src.fedoraproject.org/container/fedora-toolbox [6] https://fedoraproject.org/wiki/Changes/ToolbxReleaseBlocker [7] https://fedoraproject.org/wiki/Changes/KiwiBuiltCloudImages containers#1724 containers#1779
debarshiray
force-pushed
the
wip/rishi/DaliborKr-image_interface-00
branch
from
f858321 to
aa1b533
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 09s |
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 05s |
2 participants
A subsequent commit will switch to unmarshalling the JSON returned from
podman inspect --format json --type imagedirectly insidepodman.InspectImage()to confine the details within thepodmanpackageand make it easier to write unit tests for it. eg., it requires
tracking changes to the JSON output across different Podman versions.
Unfortunately, the JSON from
podman inspect --format json --type imageand
podman images --format jsonare considerably different and it willbe awkward to use the same implementation of the
json.Unmarshalerinterface [1] for both. One option is to have two different concrete
types separately implement
json.Unmarshalerto handle the differences inthe JSON, and then hiding these concrete types behind an
Imageinterfacethat provides access to the values parsed from the JSON.
[1] https://pkg.go.dev/encoding/json#Unmarshaler
#1724