Adding dry run option in Image.removeRecursive

[zvenigorodsky]

Partially resolving: containers/podman#27838

There's no consideration for the ignore option currently, this way we preserve current behavior and add the option

[mtrmac]

Thanks!

I don’t see any documented relationship between the Ignore option and a “dry run” feature; after this PR, there is no reason for a dry-run implementation to use the Ignore option; and there is no reason for a caller of Ignore to expect that removals wouldn’t happen.

And apart from the conceptual semantics, it doesn’t even work for the intended purpose anyway: Untag calls would still happen.

[zvenigorodsky]

Thanks!

I don’t see any documented relationship between the Ignore option and a “dry run” feature; after this PR, there is no reason for a dry-run implementation to use the Ignore option; and there is no reason for a caller of Ignore to expect that removals wouldn’t happen.

And apart from the conceptual semantics, it doesn’t even work for the intended purpose anyway: Untag calls would still happen.

Should I add another option for dry run in RemoveImageOptions? I see that there's a noPrune option but the documentation says it's only for dangling images, and about the untag I'll make changes to address that

[mtrmac]

I think you’ll also find that the current iterative logic for detecting / handling dangling images and parents does expect the removals to actually happen. So this would be a fairly major restructuring of code that claims itself to be “complex”.

[mtrmac]

I think you’ll also find that the current iterative logic for detecting / handling dangling images and parents does expect the removals to actually happen.

[zvenigorodsky]

I think you’ll also find that the current iterative logic for detecting / handling dangling images and parents does expect the removals to actually happen.

It does expect the removals to happen but from what I saw it mainly looks at the rmMap entries and the report.Removed property, which the dry run option doesn't touch.

The only calls in removeRecursive are Untag, removeContainerss and DeleteImage and we skip those calls

[mtrmac]

For a start, the danglingParent, err := parent.IsDangling(ctx) logic would be broken.

I’m not very familiar with this code and I didn’t look much further.

We can’t have a --dry-run option that under-reports its actions.

[zvenigorodsky]

For a start, the danglingParent, err := parent.IsDangling(ctx) logic would be broken.

I’m not very familiar with this code and I didn’t look much further.

We can’t have a --dry-run option that under-reports its actions.

How would it break it? if it will find a dangling parent it will run with the same options and rmMap

Would you rather I make a dedicated method for removeRecursive dry run?

[mtrmac]

In dry-run, the parent would never be dangling; after a real removal, it could.

I have no opinion on a good implementation here (and, sadly, no time to design one now — perhaps other maintainers can chime in).