Remove two GetImages functions from API by rhatdan · Pull Request #12572 · podman-container-tools/podman

rhatdan · 2021-12-10T15:20:59Z

[NO NEW TESTS NEEDED] This is just code cleanup.

The remote API has three different GetImages functions, which I believe
can be handled by just one function.

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

rhatdan · 2021-12-10T15:28:50Z

@jwhonce @flouthoc Looks like their is something special in the Docker API for handling dangling images.

I went through the code to make sure libpod, docker-compat and local all use the same code paths.

podman --remote images -a
podman images -a
DOCKER_HOST=unix:///run/user/3267/podman/podman.sock docker images -a

Are all passing exactly the same params and back the same content, but podman lists the images but the docker client does not. Any ideas?

#12567

jwhonce

Compatibility needs to be doubled checked, missing call that populated/formatted fields.

jwhonce · 2021-12-10T21:20:25Z

By skipping the call to ImageToImageSummary:

ImageSummary.VirtualSize will be incorrect.

It appears ID will no longer be prefixed with sha256:

ImageSummary.RepoDigests will no longer be formatted rather the raw data will be returned.

...

I have fixed the sha256.
VirtualSize seems to be the same between the two calls. (It is always just size).
I switched libpod output for RepoDigests to match Dockers.
I did not see any others.

flouthoc · 2021-12-11T10:04:16Z

For compat api we were ignoring manifest list I think merging this with common code would start printing manifest lists as well.

I have only looked at the code yet. But I'll need to try this out inoder to make it sure that we are not breaking any existing behavior.

This filter
filterList = append(filterList, "manifest=false")

Should remove all manifest images for the DockerAPI.

rhatdan · 2021-12-27T14:21:05Z

@vrothberg PTAL at the error.
containers/common has changed the behavior of

mkdir -p /tmp/test
podman pull alpine
podman push alpine dir:/tmp/test/
podman rmi  alpine
podman pull dir:/tmp/test

In current podman we get an image named

podman images
localhost/tmp/test                 latest      c059bfaa849c  4 weeks ago   5.87 MB

With the latest containers/common we get

podman images
<none>                <none>      c059bfaa849c  4 weeks ago   5.87 MB

I think the second option looks correct, and we should fix the pull_test.go code, but want to verify with you.

vrothberg · 2022-01-10T12:35:18Z

@rhatdan, I agree that the second one is correct. I should have check the tests in Podman and Buildah after it has merged. I will re-apply the PR in c/common and then open PRs in Podman/Buildah.

rhatdan · 2022-01-11T10:57:06Z

@containers/podman-maintainers PTAL
@vrothberg @flouthoc @giuseppe PTAL

Luap99 · 2022-01-11T11:20:58Z

I think this should be removed, @vrothberg correct?

@Luap99 I think these are dangling images from multistage builds. I dont think API should return these. But I can check what docker is returning.

I mean the change should be removed, see containers/common@134e83f
@vrothberg fixed the problem in c/common so this is no longer needed

Ah i see. Fair if that is fixed in upstream then check could be removed.

Yes, that's not needed anymore.

vrothberg

Besides @Luap99's nit LGTM. Less (redundant) code is always a great thing!

vrothberg · 2022-01-11T13:10:31Z

Yes, that's not needed anymore.

Luap99 · 2022-01-11T14:12:59Z

Please remove the change here and also remove the filterPrefix function above

We are blocking double duplicate filters. So if it has containers=external from some were else, then we don't want to add it again.

@vrothberg fixed this in c/common see the comments above, you have to rebase this to get the latest common

TomSweeneyRedHat · 2022-01-12T00:47:49Z

Could you touch up the title plase @rhatdan? I think you meant "two", not "to".

flouthoc · 2022-01-14T06:44:10Z

This the same check where @Luap99 raised concern before. If this is handled by c/common then we might not need it. But I don't think there is any blocker if we are checking for duplicates.

so LGTM

@vrothberg @Luap99 ^

Yes, the dangling-logic can/should be removed.

The dangling check is looking for dupicates. This function can be called in multiple different ways. And certain code paths and Remote API can pass in a filter with dangling=true already set, which ends up adding the dangling=true, which I am trying to prevent. If you are suggesting the common defaults to dangling=true?

The code in question was added to work around the duplicate checks in c/common. In the meantime, c/common accepts duplicates as long as they use the same value, so we don't need the custom check here anymore.

flouthoc

LGTM

openshift-ci · 2022-01-14T06:44:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [flouthoc,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

flouthoc · 2022-01-14T07:05:56Z

/hold till @vrothberg @Luap99 confirm.

[NO NEW TESTS NEEDED] This is just code cleanup. The remote API has three different GetImages functions, which I believe can be handled by just one function. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan · 2022-01-15T14:53:43Z

@vrothberg @Luap99 Mergeme.

Luap99 · 2022-01-15T16:39:43Z

/lgtm

rhatdan · 2022-01-15T19:38:57Z

/hold cancel

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2021

rhatdan force-pushed the image branch 2 times, most recently from 64a53ac to 66aef6f Compare December 10, 2021 15:24

jwhonce suggested changes Dec 10, 2021

View reviewed changes

flouthoc reviewed Dec 11, 2021

View reviewed changes

rhatdan force-pushed the image branch from 66aef6f to 9729a8d Compare December 13, 2021 20:04

rhatdan changed the title ~~Remove to GetImages functions from API~~ [wip] Remove to GetImages functions from API Dec 13, 2021

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2021

rhatdan force-pushed the image branch from 9729a8d to 2c66496 Compare December 14, 2021 17:48

rhatdan changed the title ~~[wip] Remove to GetImages functions from API~~ Remove to GetImages functions from API Dec 14, 2021

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 14, 2021

rhatdan force-pushed the image branch from 2c66496 to ed4eece Compare December 14, 2021 20:52

flouthoc linked an issue Dec 17, 2021 that may be closed by this pull request

Listing images with docker client only shows tagged images #12567

Closed

rhatdan force-pushed the image branch 7 times, most recently from 01099cb to 8598899 Compare December 22, 2021 20:57

rhatdan added the breaking-change A change that will require a full version bump, i.e. 3.* to 4.* label Dec 22, 2021

rhatdan force-pushed the image branch from 8598899 to 5d6a4fb Compare December 26, 2021 18:17

rhatdan mentioned this pull request Dec 26, 2021

fix: fix up memory usage for cgroup v2 #12703

Closed

rhatdan force-pushed the image branch from 5d6a4fb to 141f5f5 Compare December 26, 2021 18:20

rhatdan force-pushed the image branch from 141f5f5 to 0d283fc Compare January 10, 2022 18:20

flouthoc approved these changes Jan 11, 2022

View reviewed changes

Luap99 reviewed Jan 11, 2022

View reviewed changes

vrothberg reviewed Jan 11, 2022

View reviewed changes

Comment thread pkg/domain/infra/abi/images.go Outdated

Comment on lines 66 to 76

vrothberg Jan 11, 2022

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Yes, that's not needed anymore.

rhatdan force-pushed the image branch from 0d283fc to 2705562 Compare January 11, 2022 14:10

Luap99 reviewed Jan 11, 2022

View reviewed changes

rhatdan force-pushed the image branch from 2705562 to 60ab6dc Compare January 11, 2022 18:42

rhatdan changed the title ~~Remove to GetImages functions from API~~ Remove two GetImages functions from API Jan 12, 2022

rhatdan force-pushed the image branch 2 times, most recently from a759ee4 to c3d779d Compare January 12, 2022 20:02

flouthoc reviewed Jan 14, 2022

View reviewed changes

flouthoc approved these changes Jan 14, 2022

View reviewed changes

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2022

Remove two GetImages functions from API

14940a0

[NO NEW TESTS NEEDED] This is just code cleanup. The remote API has three different GetImages functions, which I believe can be handled by just one function. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan force-pushed the image branch from c3d779d to 14940a0 Compare January 14, 2022 12:38

openshift-ci Bot assigned Luap99 Jan 15, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2022

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2022

openshift-merge-robot merged commit 3c9e41b into podman-container-tools:main Jan 15, 2022

edsantiago mentioned this pull request Feb 7, 2022

podman play kube --replace : timeout #12039

Closed

github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023

github-actions Bot locked as resolved and limited conversation to collaborators Sep 21, 2023

Conversation

rhatdan commented Dec 10, 2021

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Uh oh!

rhatdan commented Dec 10, 2021

Uh oh!

jwhonce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan Dec 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Dec 27, 2021

Uh oh!

vrothberg commented Jan 10, 2022

Uh oh!

rhatdan commented Jan 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented Jan 12, 2022

Uh oh!

flouthoc Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flouthoc left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jan 14, 2022

Uh oh!

flouthoc commented Jan 14, 2022

Uh oh!

rhatdan commented Jan 15, 2022

Uh oh!

Luap99 commented Jan 15, 2022

Uh oh!

rhatdan commented Jan 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

rhatdan Dec 22, 2021 •

edited

Loading

flouthoc Jan 14, 2022 •

edited

Loading