Adding more varlink endpoints #1924
Conversation
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
baude
force-pushed
the
mroevarlinkendpoints
branch
from
a54556f to
bc83f18
Compare
cmd/podman/varlink/io.podman.varlink
Outdated
There was a problem hiding this comment.
I don't think Mount/Unmount make any sense in the context of a potentially remote API. Is there any way we can scope these so they're local-only? otherwise, I think we might want to investigate a Copy API instead.
There was a problem hiding this comment.
I happen to differ here. In a pure API sense, folks might want to mount an image and do something on the images filesystem? like a scan?
There was a problem hiding this comment.
If it's on a remote system, they can't do the scan without using our API to do it (or manually SSHing into the box in question, at which point, why not just use podman on that host?). If it goes through our API we can handle the mount/unmount ourselves.
There was a problem hiding this comment.
Sure, but the API isn't just for python, pypodman, or remote use. So someone could be developing local plugins and in other languages of course. Perhaps we agree to not expose this with our python library?
There was a problem hiding this comment.
Agree. If we're doing that, though, I think we drop the show mounts endpoint? I don't see meaning in that outside of the Podman CLI
There was a problem hiding this comment.
Id like to keep it; it is easy enough to expose. And again, we dont rightly know outside the python libraries what folks will do with it yet; if we dont want to expose it beyond the API, then we can just have @jwhonce not expose it
There was a problem hiding this comment.
Ok I see @baude Point, that doing podman mount from a import podman makes some sense. Also this would allow cockpit to do it from nodejs. If we run this over SSH, then we should just block the call as not supported,
cmd/podman/varlink/io.podman.varlink
Outdated
There was a problem hiding this comment.
What do we need this for? What does it do, list all a container's bind mounts?
There was a problem hiding this comment.
this is the equiv of podman mount with no args
baude
force-pushed
the
mroevarlinkendpoints
branch
2 times, most recently
from
42eb60a to
d6b8f78
Compare
* runlabel * checkpoint * restore * container|image exists * mount * unmount Signed-off-by: baude <[email protected]>
baude
force-pushed
the
mroevarlinkendpoints
branch
from
d6b8f78 to
5c02dda
Compare
|
bot, retest this please |
1 similar comment
|
bot, retest this please |
|
LGTM |
|
|
||
| [func ContainerCheckpoint(name: string, keep: bool, leaveRunning: bool, tcpEstablished: bool) string](#ContainerCheckpoint) | ||
|
|
||
| [func ContainerExists(name: string) int](#ContainerExists) |
There was a problem hiding this comment.
Why is this better than calling GetContainer() and checking for an error?
There was a problem hiding this comment.
It is provided for convenience to match the CLI. It also returns a 0,1, other return code. Again, we have to think beyond the consumption of the API by python.
There was a problem hiding this comment.
I don't see this as a Python issue at all, I can call the same thing from nodejs. Now we have two ways of determining if a container exists.
If GetContainer(foobar) returns an error constant that image does not exist, we don't need this other interface.
|
|
||
| [func HistoryImage(name: string) ImageHistory](#HistoryImage) | ||
|
|
||
| [func ImageExists(name: string) int](#ImageExists) |
There was a problem hiding this comment.
Why is this better than GetImage() and checking for an error?
There was a problem hiding this comment.
Probably from an API point of view, it is not. The exists calls were added to differentiate between errors of no exist versus , you can not check if container exists.
So i the GetImage returns an error code that can be determined to be Container does not exist versus permission denied, no need for the new API.
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
6 participants
Signed-off-by: baude [email protected]