Add podman system prune and info commands by rhatdan · Pull Request #2252 · containers/podman

rhatdan · 2019-02-01T01:42:04Z

We are missing the equivalence of the docker system commands

This patch set adds podman system prune

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

openshift-ci-robot · 2019-02-01T01:42:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [rhatdan]

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

baude · 2019-02-01T01:50:03Z

cmd/podman/system_prune.go

bonus points for using the adapter.Runtime!

baude · 2019-02-01T01:50:48Z

would really like some tests if that is possible

mheon · 2019-02-01T01:50:54Z

cmd/podman/system_prune.go

I don't think docker system prune actually prunes volumes. The description says exclusively containers, images, networks

It does, see https://docs.docker.com/engine/reference/commandline/system_prune/

There's a --volumes flag.

rhatdan · 2019-02-01T16:10:26Z

/test images

rhatdan · 2019-02-01T16:10:58Z

@vrothberg @mheon @baude @umohnani8 @giuseppe @TomSweeneyRedHat PTAL
@baude Now has tests.

mheon · 2019-02-01T16:13:35Z

cmd/podman/system_prune.go

Should we be printing IDs here?

volumePrune should print the ids that it is removing.

rhatdan · 2019-02-01T18:34:22Z

/test images

mheon · 2019-02-01T19:42:39Z

LGTM

vrothberg

Nice PR! Just some minor comments regarding --force and making the manpage and help message a bit clearer.

vrothberg · 2019-02-04T08:01:42Z

cmd/podman/system_prune.go

s/remove/Remove/ for consistency

Fixed these all in the second patch.

vrothberg · 2019-02-04T08:02:14Z

cmd/podman/system_prune.go

"data" versus "systems"?

Data is correct. systems was incorrect.

vrothberg · 2019-02-04T08:04:06Z

cmd/podman/system_prune.go

Can we add - all volumes if the flag is set?

vrothberg · 2019-02-04T08:04:55Z

cmd/podman/system_prune.go

s/err/errPruneContainers/

I am not sure if we should use --force here as it should only impact the cli prompt.

vrothberg · 2019-02-04T08:05:32Z

cmd/podman/system_prune.go

s/err1/err/PruneImages/ ... just to make the code easier to brain-parse

Ok I changed to use lasterr to make it clearer.

vrothberg · 2019-02-04T08:06:13Z

docs/podman-system-prune.1.md

Copy artifact?

vrothberg · 2019-02-04T08:07:50Z

docs/podman-system-prune.1.md

Can we define what we mean by systems (i.e., containers, images, build cache)? Same applies to the help message. Currently, it's hard to figure out which data is removed without actually running the command.

This is a global replace issue. System should be Images
Fixed

giuseppe · 2019-02-05T09:37:36Z

LGTM

TomSweeneyRedHat · 2019-02-05T12:17:35Z

cmd/podman/pod_unpause.go

s/Anpause/Unpause/

TomSweeneyRedHat · 2019-02-05T12:23:48Z

docs/podman-info.1.md

Should this be podman-info?

TomSweeneyRedHat · 2019-02-05T12:24:31Z

docs/podman-info.1.md

Should this have it's own page, even if it's mostly a cut/paste job?

No they are identical, This is what we do with all of the podman container commands

podman-container-ps -> podman-ps.
for example. I think we should add the alternate names to the linked images though.

TomSweeneyRedHat · 2019-02-05T12:27:32Z

docs/podman-system.1.md

I don't think the podman-system-info.1.md exists and I don't see it in this PR.

TomSweeneyRedHat · 2019-02-05T12:28:42Z

docs/podman.1.md

I think you should add podman-system-info and podman-system-prune here as well.

We don't add any subcommands on the main man page?

podman container prune is not there so

TomSweeneyRedHat

Any plans to implement podman system df or podman system events to match Docker? Even if they're no-ops?

rhatdan · 2019-02-05T13:29:43Z

Yes we need to add df and events, althoug podman-system-events is just another way of calling podman events, which also does not exist yet.

rh-atomic-bot · 2019-02-05T18:34:21Z

☔ The latest upstream changes (presumably #2227) made this pull request unmergeable. Please resolve the merge conflicts.

We are missing the equivalence of the docker system commands This patch set adds `podman system prune` and `podman system info` Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

We have no consistancy in out option usages and descritions on whether or not the first letter should be capatalized. This patch forces them all to be capatilized. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

openshift-merge-robot · 2019-02-06T14:49:44Z

/retest

vrothberg · 2019-02-06T16:14:04Z

/test validate

baude · 2019-02-06T21:22:51Z

/lgtm

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2019

openshift-ci-robot requested review from TomSweeneyRedHat and mheon February 1, 2019 01:42

openshift-ci-robot added the size/L label Feb 1, 2019

rhatdan force-pushed the system branch from cb4c283 to 7837c0d Compare February 1, 2019 01:42

baude reviewed Feb 1, 2019

View reviewed changes

cmd/podman/system_prune.go Outdated

Copy link

Member

baude Feb 1, 2019

Choose a reason for hiding this comment

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

bonus points for using the adapter.Runtime!

mheon reviewed Feb 1, 2019

View reviewed changes

rhatdan force-pushed the system branch 2 times, most recently from b5c0f01 to 63017fd Compare February 1, 2019 14:12

mheon reviewed Feb 1, 2019

View reviewed changes

vrothberg reviewed Feb 4, 2019

View reviewed changes

rhatdan force-pushed the system branch 2 times, most recently from bd4c286 to fb8a731 Compare February 4, 2019 18:48

TomSweeneyRedHat reviewed Feb 5, 2019

View reviewed changes

rhatdan force-pushed the system branch from fb8a731 to 4a2edb5 Compare February 5, 2019 18:14

Add podman system prune and info commands

74d984e

We are missing the equivalence of the docker system commands This patch set adds `podman system prune` and `podman system info` Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

Capatilize all usage and descriptions

0830bb9

We have no consistancy in out option usages and descritions on whether or not the first letter should be capatalized. This patch forces them all to be capatilized. Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan force-pushed the system branch from 4a2edb5 to 0830bb9 Compare February 5, 2019 18:42

openshift-ci-robot assigned baude Feb 6, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2019

openshift-merge-robot merged commit c735072 into containers:master Feb 6, 2019

This was referenced Feb 6, 2019

Fix volume handling in podman #2229

Merged

Add support to migrate containers #2272

Merged

Migrate to cobra CLI #2274

Merged

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 27, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023

Conversation

rhatdan commented Feb 1, 2019

Uh oh!

openshift-ci-robot commented Feb 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Feb 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrothberg Feb 1, 2019 • 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 Feb 1, 2019

Uh oh!

rhatdan commented Feb 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Feb 1, 2019

Uh oh!

mheon commented Feb 1, 2019

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!

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!

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!

giuseppe commented Feb 5, 2019

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!

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 Feb 1, 2019 •

edited

Loading