gabrieldemarmiesse / python-on-whales

An awesome Python wrapper for an awesome Docker CLI!
MIT License
557 stars 102 forks source link

Implement support for podman pods #544

Closed LewisGaul closed 5 months ago

LewisGaul commented 9 months ago

This feature only applies for podman, not docker.

Questions/notes:

TODO:

Follow-up:

LewisGaul commented 9 months ago

@gabrieldemarmiesse any thoughts on what needs doing here, with regards to docs and tests?

gabrieldemarmiesse commented 8 months ago

How should we encorporate this in the docs, since the commands are current listed as "docker " but docker does not support "docker pod"?

We can choose the name of web pages, do instead of naming it "docker ..." like the other commands, we could just name it "podman pod"

I have implemented Container.pod to return the string pod ID rather than the Pod object

Good call, I still haven't decided what to do here.

Is it the right choice to support --latest and --all to podman pod {start,restart,stop,kill,pause,unpause}?

I think so yes. We want python-on-whales to be familiar to users who started with the CLI (in this case the podman CLI).

The rest of the review will be done, no worries, I'm just finding the time here and there

gabrieldemarmiesse commented 8 months ago

Is it the right choice to support args like pod_id_file in PodCLI.create()?

I don't see what is special about it. Maybe I'm missing something?

gabrieldemarmiesse commented 8 months ago

Food for thought, I wonder if we should have a sentinel value for "all objects" that we can use in this PR but also as default value in some other places. It might seems strange to have containers=None mean all containers (since it's the default value). We could also have a sentinel for latest.

LewisGaul commented 5 months ago

Is it the right choice to support args like pod_id_file in PodCLI.create()?

I don't see what is special about it. Maybe I'm missing something?

Only that it's probably easier to get the ID of the pod when using PoW than when calling the CLI so less use for having podman write it to a file for you (tbh this seems like a slightly odd CLI arg to me). But happy to just leave it for consistency with the CLI.

LewisGaul commented 5 months ago

@gabrieldemarmiesse This should be ready for review now (EDIT: now I've fixed the bad merge)