Fix artifact inspect to handle multiple artifacts #27328
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Doesn't #27329 do this as well |
|
Yes the other PR is a follow on to this, lets get this merged first. Just fixed your comments from the other PR. |
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| Expect(output).To(BeValidJSON()) | ||
|
|
||
| // Verify it's an array with 2 elements | ||
| var artifacts []map[string]interface{} |
There was a problem hiding this comment.
Should use the actual entities.ArtifactInspectReport type. We make no promises our keys are strings, so this is going to break down the road.
There was a problem hiding this comment.
This is getting string output from the command line, so I think it will have to be strings.
There was a problem hiding this comment.
It's deserializing an array of objects we serialized ourselves in the output logic? It's guaranteed to be a []entities.ArtifactInspectReport
Currently, podman artifact inspect only inspects the first artifact specified on the command line (args[0]), ignoring any additional artifacts. This differs from podman image inspect which properly handles multiple images. This change fixes the inspect function to iterate through all provided artifact names, collecting successful inspections and errors separately. Results are printed as a JSON array, matching the behavior of other inspect commands. Changes: - Loop through all args instead of only inspecting args[0] - Collect inspect results in a slice - Print all successful inspections - Add test for multiple artifact inspection Fixes: containers/podman#XXXXX Co-authored-by: Cursor AI <[email protected]> Signed-off-by: Daniel Walsh <[email protected]> Signed-off-by: Daniel J Walsh <[email protected]>
|
I intentionally did not allow inspecting of multiple artifacts because I don't like the user flow for it. Anyways, you are changing the output type here correct? We cannot do that in podman 5? |
| continue | ||
| } | ||
| // Print all successful inspections | ||
| if err := utils.PrintGenericJSON(data); err != nil { |
There was a problem hiding this comment.
If it's >1 we should be making an array of them and serializing that.
|
I am fine with that but it was documented incorrectly. I will fix the documentation and go back to a single artifact. |
|
I'm not opposed to multiple inspect. It matches the behavior of every other inspect we have. |
|
Well I am going to close this PR and fix this in --format PR, with a single inspect. We can handle multiple inspects in a different PR, if people consider this important. I only worked on this because of the documentation. I personally do not see the need for multiple inspects. |
3 participants
Currently, podman artifact inspect only inspects the first artifact specified on the command line (args[0]), ignoring any additional artifacts. This differs from podman image inspect which properly handles multiple images.
This change fixes the inspect function to iterate through all provided artifact names, collecting successful inspections and errors separately. Results are printed as a JSON array, matching the behavior of other inspect commands.
Changes:
Fixes: containers/podman#XXXXX
Does this PR introduce a user-facing change?