Refactor podman python varlink bindings #748

jwhonce · 2018-05-11T00:57:25Z

More pythonic
Leverage context managers to help with socket leaks
Add system unittest's
Add image unittest's
Add container unittest's
Add models for system, containers and images, and their collections
Add helper functions for datetime parsing/formatting
GetInfo() implemented will need updated when actual code on master
setuptools support added for packaging

Todo:

coverage
documentation
remove test/varlink

Signed-off-by: Jhon Honce jhonce@redhat.com

mheon · 2018-05-11T01:03:26Z

@jwhonce I'll give it a review, but I warn you it's been years since I did serious work in Python 😄

wking

I've only made it up to libs/images.py, but here are some initial suggestions. I'm not a maintainer, so feel free to ignore anything that doesn't seem useful ;).

wking · 2018-05-11T04:54:48Z

contrib/pylibpod/README.md

Should we just call it libpod, and have it's Python-ness implied by it's presence in PyPI? import libpod, called from Python, is not going to pull in a Go library by mistake ;).

@wking I cargo-culted that from pypi where there are a lot of libraries that are so named. I would be game for calling the git directory pylibpod and the python module libpod. @baude votes?

wking · 2018-05-11T04:55:55Z

contrib/pylibpod/README.md

This can be a relative link:

See [libpod](../../).

wking · 2018-05-11T04:57:12Z

contrib/pylibpod/pylibpod/__init__.py

Can we use SemVer and start with 0.1.0?

Agreed, Done.

wking · 2018-05-11T04:58:14Z

contrib/pylibpod/pylibpod/__init__.py

I haven't seen __title__ before. Can you link to docs or such discussing it? I'd expect the package name to be sufficient without repeating it here.

@wking pkutils use it. If the final packager we use doesn't support this then I'll remove it.

@wking pkutils use it.

I've filed reubano/pkutils#2 to make it optional ;).

wking · 2018-05-11T05:01:06Z

contrib/pylibpod/pylibpod/__init__.py

I find these imports easier to maintain if you put each imported symbol on its own line:

from .libs.errors import ( ContainerNotFound, ErrorOccurred, ImageNotFound, RuntimeError, )

That way it's easier to use your editor to alphabetize and you don't have to worry about re-wrapping as you add/remove symbols.

Alternatively, you could stay DRY by defining __all__ in these subpackages and then using a wildcard import here:

from .libs.errors import *

@wking My IDE (atom/python-ide) already sorts imported objects, unfortunately the formatter won't keep them as a list. I'll take a todo to research this. IDE too productive to dump. :)

I missed I had all vs __all__ defined. Fixed that.

wking · 2018-05-11T05:52:38Z

contrib/pylibpod/pylibpod/libs/containers.py

Is this ellipsis a TODO?

wking · 2018-05-11T05:59:33Z

contrib/pylibpod/pylibpod/libs/containers.py

You have 'returncode' here and exitcode below. POSIX isn't always consistent about this, but they use "exit status" in several places (e.g. here and here). Maybe we should use:

Wait for the container to finish and return its exit status.

here?

I chose returncode here because that is what the subprocess module uses.

wking · 2018-05-11T06:00:55Z

contrib/pylibpod/pylibpod/libs/containers.py

This seems like something that is going to need arguments (e.g. whether or not to follow a stream, or whether to only get the last n lines). But maybe those aren't supported by the varlink API yet.

@wking Correct, once we have streaming there will be changes. I added *args, **kwargs to better future proof this method.

wking · 2018-05-11T06:02:16Z

contrib/pylibpod/pylibpod/libs/containers.py

Do you mean to pass *args and **kwargs through here?

@wking They are on the method signature waiting on this varlink method to be implemented. A bit of future-proofing.

wking · 2018-05-11T06:04:00Z

contrib/pylibpod/pylibpod/libs/containers.py

If data contains an id key, why bother with _id and __init__'s separate id argument/

@wking I had intended it as a check that the data passed in matched the requested id. I'm on the fence whether this paranoia is worth the hassle. @baude thoughts?

giuseppe · 2018-05-11T13:02:43Z

Makefile

is this change needed?

I removed the pushd/popd calls.

baude · 2018-05-11T14:05:48Z

contrib/pylibpod/pylibpod/client.py

add model for consistency

baude · 2018-05-11T14:08:35Z

contrib/pylibpod/pylibpod/libs/containers.py

does it make sense to combine these two into a return? dealers choice

@baude I've kept them separate for two reasons:

the client socket connection is open for the duration of the with statement. So I've coded that to be the minimum number statements for the day when we go remote. processes() could have yielded within the with, but then the connection to podman would be open until the caller exhausted the list of processes.

I personally don't like the style of method()[0] unless the method call is trivial. :)

baude · 2018-05-11T14:13:04Z

@jwhonce big undertaking, good work!

jwhonce · 2018-05-11T19:10:10Z

@wking @baude Thanks for taking the time for the initial review!

mheon · 2018-05-11T21:01:54Z

@jwhonce Gitvalidation is complaining about whitespace errors in one of your commits

rhatdan · 2018-05-12T09:59:37Z

contrib/pylibpod/test/crio_testcase.py

Why is this called CrioTestCase? Does this require Crio?

rhatdan · 2018-05-12T10:01:55Z

Jhon great work, love all of the tests. Only question is why call this stuff Crio?

jwhonce · 2018-05-12T21:18:39Z

@rhatdan History. Named changed to reflect current usage.

TomasTomecek · 2018-05-13T06:18:41Z

Really nice work! Will this be a dedicated library?

wking · 2018-05-13T15:09:37Z

contrib/pylibpod/LICENSE.txt

This paragraph should not be indented. And you might want to just copy the root LICENSE file here anyway.

wking · 2018-05-13T15:11:00Z

contrib/pylibpod/README.md

I think you want a terminal . here.

wking · 2018-05-13T15:15:10Z

contrib/pylibpod/README.md

This should be:

```console $ python3 setup.py clean -a && python3 setup.py bdist_wheel ```

or

```sh python3 setup.py clean -a && python3 setup.py bdist_wheel ```

The python syntax highlighting is for content that can be parsed as valid Python, but your line is POSIX shell syntax invoking Python.

Yup, my editor autofilled something wrong and then I corrected it to something wrong. :)

mheon · 2018-05-14T15:11:16Z

contrib/pylibpod/test/test_libs.py

Typo: hanlded -> handled

mheon

Weak LGTM - I'm not comfortable enough with Python to really do a proper review.

rhatdan · 2018-05-14T19:42:54Z

My biggest request is the library needs to be named podman

import podman

No one knows what libpod is, and libpod means something different. I would figure libpod python bindings would give me access to some of the internals or databases of libpod.

Change the name to podman and we can merge this. (Although I like the idea of calling it podcast, but that would confuse users.)

jwhonce · 2018-05-15T01:31:41Z

@rh-atomic-bot retry

giuseppe · 2018-05-15T08:56:27Z

bot, please retest

baude · 2018-05-15T13:04:52Z

bot, retest this please

mheon · 2018-05-15T13:42:35Z

@jwhonce Are the Travis failures flakes, or actual misconfiguration? It looks like it's throwing errors in the Python tests for the varlink bindings?

- More pythonic - Leverage context managers to help with socket leaks - Add system unittest's - Add image unittest's - Add container unittest's - Add models for system, containers and images, and their collections - Add helper functions for datetime parsing/formatting - GetInfo() implemented - Add support for setuptools - Update documentation - Support for Python 3.4-3.6 Signed-off-by: Jhon Honce <jhonce@redhat.com>

jwhonce · 2018-05-15T22:35:55Z

@mheon Python version support issue. Code downgraded for Python 3.4 support.

mheon · 2018-05-15T23:36:08Z

Tests seem to be green. I'll leave final decision on merge to @baude as our resident varlink expert.

baude · 2018-05-16T13:35:19Z

@rh-atomic-bot r+

rh-atomic-bot · 2018-05-16T13:35:19Z

📌 Commit 814c6af has been approved by baude

rh-atomic-bot · 2018-05-16T14:01:13Z

⌛ Testing commit 814c6af with merge 1aaf8df...

rh-atomic-bot · 2018-05-16T14:39:48Z

☀️ Test successful - status-papr
Approved by: baude
Pushing 1aaf8df to master...

jwhonce requested review from baude and mheon May 11, 2018 00:57

jwhonce self-assigned this May 11, 2018

jwhonce added the enhancement label May 11, 2018

wking reviewed May 11, 2018

View reviewed changes

giuseppe reviewed May 11, 2018

View reviewed changes

baude reviewed May 11, 2018

View reviewed changes

jwhonce mentioned this pull request May 11, 2018

varlink: InspectImage does not serialize the metadata #742

Closed

jwhonce force-pushed the wip/python_client branch 3 times, most recently from f673797 to 0666aba Compare May 12, 2018 05:14

rhatdan reviewed May 12, 2018

View reviewed changes

contrib/pylibpod/test/crio_testcase.py Outdated

Copy link

Member

rhatdan May 12, 2018

Choose a reason for hiding this comment

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

Why is this called CrioTestCase? Does this require Crio?

jwhonce force-pushed the wip/python_client branch from 0666aba to cfa81f2 Compare May 12, 2018 21:13

wking reviewed May 13, 2018

View reviewed changes

mheon reviewed May 14, 2018

View reviewed changes

contrib/pylibpod/test/test_libs.py Outdated

Copy link

Member

mheon May 14, 2018

Choose a reason for hiding this comment

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

Typo: hanlded -> handled

mheon approved these changes May 14, 2018

View reviewed changes

jwhonce force-pushed the wip/python_client branch from cfa81f2 to 4d689ba Compare May 14, 2018 15:14

jwhonce force-pushed the wip/python_client branch from 4d689ba to 348f920 Compare May 14, 2018 21:58

jwhonce changed the title ~~Refactor libpod python varlink bindings~~ Refactor podman python varlink bindings May 14, 2018

jwhonce force-pushed the wip/python_client branch from 348f920 to c31a69b Compare May 14, 2018 22:18

mheon mentioned this pull request May 14, 2018

Remove old varlink tests #766

Closed

jwhonce force-pushed the wip/python_client branch 5 times, most recently from 2144a03 to aa21e08 Compare May 15, 2018 01:01

jwhonce force-pushed the wip/python_client branch from aa21e08 to 814c6af Compare May 15, 2018 22:31

rh-atomic-bot closed this in 1aaf8df May 16, 2018

wking mentioned this pull request May 16, 2018

Makefile: Respect GOBIN #774

Closed

TomasTomecek mentioned this pull request May 17, 2018

implement libpod/podman as a new backend user-cont/conu#200

Closed

jwhonce deleted the wip/python_client branch January 28, 2020 21:16

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

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

Refactor podman python varlink bindings #748

Refactor podman python varlink bindings #748

Uh oh!

Conversation

jwhonce commented May 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mheon commented May 11, 2018

Uh oh!

wking 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!

jwhonce May 11, 2018 • 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!

jwhonce May 11, 2018 • 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!

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!

jwhonce May 11, 2018 • 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!

baude commented May 11, 2018

Uh oh!

jwhonce commented May 11, 2018

Uh oh!

jwhonce commented May 11, 2018 •

edited

Loading

jwhonce May 11, 2018 •

edited

Loading

jwhonce May 11, 2018 •

edited

Loading

jwhonce May 11, 2018 •

edited

Loading

jwhonce commented May 15, 2018 •

edited

Loading

giuseppe commented May 15, 2018 •

edited

Loading