Skip to content

Fix duplicate results in podman images <image> #25783

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Fix duplicate results in podman images <image> #25783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions cmd/podman/images/list.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"os"
"regexp"
"slices"
"strings"
"time"
Expand Down Expand Up @@ -128,10 +129,27 @@ func images(cmd *cobra.Command, args []string) error {
return err
}

imgs, err := sortImages(summaries)
reference := ""
for _, filter := range listOptions.Filter {
if strings.HasPrefix(filter, "reference=") {
reference = strings.TrimPrefix(filter, "reference=")
Comment on lines +134 to +135
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't the regular filter code allow more than one reference? SO I think this needs to handle all not just the first match.

Also you should use CutPrefix()

if strings.HasPrefix(reference, "sha256:") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are digests filtered out here?

reference = ""
break
}
reference, _, _ = tokenRepoTag(reference)
if strings.Contains(reference, "<none>") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be an == not contains? But again it is not clear to me why this is done at all.

This whole section should have proper comments what is being filtered here.

reference = ""
}
break
}
}

imgs, err := sortImages(summaries, reference)
if err != nil {
return err
}

switch {
case report.IsJSON(listFlag.format):
return writeJSON(imgs)
Expand Down Expand Up @@ -214,9 +232,16 @@ func writeTemplate(cmd *cobra.Command, imgs []imageReporter) error {
return rpt.Execute(imgs)
}

func sortImages(imageS []*entities.ImageSummary) ([]imageReporter, error) {
imgs := make([]imageReporter, 0, len(imageS))
func sortImages(imageS []*entities.ImageSummary, reference string) ([]imageReporter, error) {
var err error
var referenceRegex *regexp.Regexp
if reference != "" {
referenceRegex, err = regexp.Compile(reference + "$")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this effectively a HasSuffix() match, why do we need a regex for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, a suffix match is still not at all great: example.com/foo matches notexample.com/foo.

As a first guess, I agree with @Luap99 that this (whatever “this” is[1]) should be done in libimage. I don’t know how, maybe a new “return only matching names” option to ListImageOptions, maybe something completely different.

The principle, as I understand it, is that libimage knows exactly what matched and how. That information is ~lost here, and we are reduced to guessing; and if we ever change libimage, this will get out of sync.

[1] “whatever this is”: I do expect that this might look harder to do inside libimage, because all the various corner cases will be visible. E.g. what should happen on reference!=… filters?? But, to me, that’s an even stronger reason to do it in libimage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, Now I understand why. I will do it in libimage. And I will keep PR as a draft for tests and vendoring of c/common.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libimage PR: containers/common#2413

if err != nil {
return nil, err
}
}
imgs := make([]imageReporter, 0, len(imageS))
for _, e := range imageS {
var h imageReporter
if len(e.RepoTags) > 0 {
Expand All @@ -237,7 +262,12 @@ func sortImages(imageS []*entities.ImageSummary) ([]imageReporter, error) {
// Note: we only want to display "<none>" if we
// couldn't find any tagged name in RepoTags.
if len(tagged) > 0 {
imgs = append(imgs, tagged...)
for _, i := range tagged {
if reference != "" && !referenceRegex.MatchString(i.Repository) {
continue
}
imgs = append(imgs, i)
}
} else {
imgs = append(imgs, untagged[0])
}
Expand Down
12 changes: 12 additions & 0 deletions test/e2e/images_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -547,4 +547,16 @@ LABEL xyz="bar"
Expect(session.OutputToString()).To(ContainSubstring("test-abc-xyz"))
})

It("podman images <image> show duplicate images", func() {
containerFile := "FROM quay.io/libpod/alpine:latest"
imageName := "exampletestimage"
podmanTest.BuildImage(containerFile, imageName, "false")
Comment on lines +551 to +553
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can use podman tag instead which seems simpler


result := podmanTest.PodmanExitCleanly("images", imageName)

Expect(result.OutputToStringArray()).To(HaveLen(2))
Expect(result.OutputToString()).To(ContainSubstring(imageName))
Expect(result.OutputToString()).ToNot(ContainSubstring("alpine"))
})

})