Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix TypeError in container's wait method #187

Closed
wants to merge 2 commits into from

Conversation

mrbazzan
Copy link
Contributor

@mrbazzan mrbazzan commented Jul 12, 2022

For the doc change: list of string does not work with environment argument

Fixes No.3 in #184, .wait() returns an integer not a dictionary

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 12, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mrbazzan
To complete the pull request process, please assign mheon after the PR has been reviewed.
You can assign the PR to them by writing /assign @mheon in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

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

@mrbazzan mrbazzan changed the title Fix Fix TypeError in container's wait method Jul 12, 2022
@rhatdan
Copy link
Member

rhatdan commented Jul 12, 2022

LGTM
@mwhahaha @jwhonce @cdoern PTAL

@@ -77,7 +77,7 @@ def run(
if log_type in ("json-file", "journald"):
log_iter = container.logs(stdout=stdout, stderr=stderr, stream=True, follow=True)

exit_status = container.wait()["StatusCode"]
exit_status = container.wait()
if exit_status != 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

how does this check still work if you remove the ["StatusCode"] aspect? now you are checking if DICT != 0

Copy link
Contributor Author

@mrbazzan mrbazzan Jul 12, 2022

Choose a reason for hiding this comment

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

The return value from container.wait() is an integer not a dict, right?(unless I'm missing something...)

Copy link
Member

Choose a reason for hiding this comment

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

def wait(self, **kwargs) -> Dict[Literal["StatusCode", "Error"], Any]:

Looks like to original code is correct

Choose a reason for hiding this comment

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

Wait wait wait, there is definitely an issue here.
By default, wait() in podman-py uses the libpod operation ContainerwaitLibPod, which returns a JSON-encoded integer. For that case, the type annotation

def wait(self, **kwargs) -> Dict[Literal["StatusCode", "Error"], Any]:

isn't correct. That annotation matches the podman compat API, where ContainerWait returns a JSON object with the expected Keys Error and StatusCode.
I just discovered podman-py and don't know the code base enough yet to fix it myself, but would recommend to look into that issue again. I got here having experienced the same issue as in #201

Copy link
Member

Choose a reason for hiding this comment

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

@cdoern PTAL

@rhatdan rhatdan closed this Aug 9, 2022
@jonathanunderwood
Copy link
Contributor

@rhatdan wondering why this was closed, as the issue is still present. As @sylencecc points out, the proposed fix is correct, but the type annotation for the return type ofwait() is wrong and also needs fixing.

@rhatdan
Copy link
Member

rhatdan commented Jan 17, 2023

I don't remember but might have been lack of activity.

Care to open a new PR based on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants