cmd/list, pkg/podman: Eliminate getImages() by doing everything in podman.GetImages()#1772
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the image retrieval and processing mechanism by centralizing the logic for de-duplication and name flattening within the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
This is a step towards eliminating or reducing the code in getImages() by doing everything or more in podman.GetImages(). If nothing else, this removes the need to export Image.FlattenNames() from the podman package. The getImages() function originated as listContainers(), when it invoked 'podman images --filter label=...' through podman.GetImages() separately for each label used to identify Toolbx images, and then joined and sorted the results. This changed in commit 2369da5 when getImages() started invoking 'podman images' only once through podman.GetImages() to get all available images, and then filtered them itself based on the labels. Before this change in commit 2369da5, it probably made some sense to keep podman.GetImages() only as a thin wrapper to invoke 'podman images' with different options, and to do everything else in getImages(). However, since then more is being done inside the podman package (eg., unmarshalling the 'podman images' JSON in commit 5f324d5 and flattening the images in commit 6aab0a6), and getImages() is the only caller of podman.GetImages(). Therefore, it looks awkward to have the code to get all Toolbx images split across two functions in different packages. containers#1724 containers#1772
debarshiray
force-pushed
the
wip/rishi/cmd-list-pkg-podman-getImages
branch
from
14c376e to
616501d
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 the image fetching logic by moving the image name flattening and de-duplication from cmd/list.go into pkg/podman/podman.go. This is a good step towards centralizing the image handling logic. However, I found a critical issue in the implementation where the refactored function returns the original unprocessed data instead of the processed results, which defeats the purpose of the change.
This is a step towards eliminating or reducing the code in getImages() by doing everything or more in podman.GetImages(). If nothing else, this removes the need to export Image.FlattenNames() from the podman package. The getImages() function originated as listContainers(), when it invoked 'podman images --filter label=...' through podman.GetImages() separately for each label used to identify Toolbx images, and then joined and sorted the results. This changed in commit 2369da5 when getImages() started invoking 'podman images' only once through podman.GetImages() to get all available images, and then filtered them itself based on the labels. Before this change in commit 2369da5, it probably made some sense to keep podman.GetImages() only as a thin wrapper to invoke 'podman images' with different options, and to do everything else in getImages(). However, since then more is being done inside the podman package (eg., unmarshalling the 'podman images' JSON in commit 5f324d5 and flattening the images in commit 6aab0a6), and getImages() is the only caller of podman.GetImages(). Therefore, it looks awkward to have the code to get all Toolbx images split across two functions in different packages. containers#1724 containers#1772
debarshiray
force-pushed
the
wip/rishi/cmd-list-pkg-podman-getImages
branch
from
616501d to
843208f
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 11s |
Subsequent commits will use isToolbx() for image labels. So, it can't be in a file that's specific to containers. containers#1724 containers#1772
This is a step towards eliminating or reducing the code in getImages() by doing everything or more in podman.GetImages(). If nothing else, this removes the need to keep a copy of the labels used to identify Toolbx images for getImages(). The getImages() function originated as listContainers(), when it invoked 'podman images --filter label=...' through podman.GetImages() separately for each label used to identify Toolbx images, and then joined and sorted the results. This changed in commit 2369da5 when getImages() started invoking 'podman images' only once through podman.GetImages() to get all available images, and then filtered them itself based on the labels. Before this change in commit 2369da5, it probably made some sense to keep podman.GetImages() only as a thin wrapper to invoke 'podman images' with different options, and to do everything else in getImages(). However, since then more is being done inside the podman package (eg., unmarshalling the 'podman images' JSON in commit 5f324d5 and flattening the images in commit 6aab0a6), and getImages() is the only caller of podman.GetImages(). Therefore, it looks awkward to have the code to get all Toolbx images split across two functions in different packages. containers#1724 containers#1772
This is a step towards eliminating the code in getImages() by doing everything in podman.GetImages(). This removes the need to export the podman.ImageSlice type from the podman package. The getImages() function originated as listContainers(), when it invoked 'podman images --filter label=...' through podman.GetImages() separately for each label used to identify Toolbx images, and then joined and sorted the results. This changed in commit 2369da5 when getImages() started invoking 'podman images' only once through podman.GetImages() to get all available images, and then filtered them itself based on the labels. Before this change in commit 2369da5, it probably made some sense to keep podman.GetImages() only as a thin wrapper to invoke 'podman images' with different options, and to do everything else in getImages(). However, since then more is being done inside the podman package (eg., unmarshalling the 'podman images' JSON in commit 5f324d5 and flattening the images in commit 6aab0a6), and getImages() is the only caller of podman.GetImages(). Therefore, it looks awkward to have the code to get all Toolbx images split across two functions in different packages. containers#1724 containers#1772
debarshiray
changed the title
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 20s |
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 16s |
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 19s |
The getImages() function originated as listContainers(), when it invoked 'podman images --filter label=...' through podman.GetImages() separately for each label used to identify Toolbx images. The label was specified using the 'args' parameter. This changed in commit 2369da5 when getImages() started invoking 'podman images' only once through podman.GetImages() to get all available images, with the 'args' parameter set to a fixed value. Then in commit 6aab0a6 the 'args' parameter became unused. containers#1772
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 18s |
debarshiray
force-pushed
the
wip/rishi/cmd-list-pkg-podman-getImages
branch
from
dd54eba to
4cd38d3
Compare
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 55s |
2 participants
These are steps towards eliminating the code in
getImages()by doing everything inpodman.GetImages().The
getImages()function originated aslistContainers(), when it invokedpodman images --filter label=...throughpodman.GetImages()separately for each label used to identify Toolbx images, and then joined and sorted the results. This changed in commit 2369da5 whengetImages()started invokingpodman imagesonly once throughpodman.GetImages()to get all available images, and then filtered them itself based on the labels.Before this change in commit 2369da5, it probably made some sense to keep
podman.GetImages()only as a thin wrapper to invokepodman imageswith different options, and to do everything else ingetImages(). However, since then more is being done inside thepodmanpackage (eg., unmarshalling thepodman imagesJSON in commit 5f324d5 and flattening the images in commit 6aab0a6), andgetImages()is the only caller of podman.GetImages().Therefore, it looks awkward to have the code to get all Toolbx images split across two functions in different packages.
#1724