Skip to content

Conversation

@dklimpel
Copy link
Contributor

@dklimpel
Copy link
Contributor Author

/assign vrothberg

@vrothberg
Copy link
Member

@jwhonce PTAL

@jwhonce
Copy link
Member

jwhonce commented Oct 15, 2025

/approve

@jwhonce jwhonce requested a review from Copilot October 15, 2025 16:48
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes typing issues in the podman.domain.containers_run module by resolving type inconsistencies and adding type ignore comments where necessary. The changes enable removal of this module from mypy's ignore list.

Key changes:

  • Corrected return type annotation to use Generator[bytes, None, None] instead of Generator[str, None, None]
  • Renamed image variable to image_id for clarity after extracting the ID from Image objects
  • Added type ignore comments for dynamic attribute access

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
pyproject.toml Removed podman.domain.containers_run from mypy ignore list
podman/domain/containers_run.py Fixed return type annotations and variable naming, added type ignore comments for unresolved dynamic attributes

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

remove: bool = False,
**kwargs,
) -> Union[Container, Union[Generator[str, None, None], Iterator[str]]]:
) -> Union[Container, Union[Generator[bytes, None, None], Iterator[str]]]:
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

Inconsistent return types in Union: Generator yields bytes but Iterator yields str. Both should yield the same type for consistency. Consider changing to Union[Container, Union[Generator[bytes, None, None], Iterator[bytes]]].

Suggested change
) -> Union[Container, Union[Generator[bytes, None, None], Iterator[str]]]:
) -> Union[Container, Union[Generator[bytes, None, None], Iterator[bytes]]]:

Copilot uses AI. Check for mistakes.
raise ContainerError(container, exit_status, command, image_id, log_iter)

return log_iter if kwargs.get("stream", False) or log_iter is None else b"".join(log_iter)
return log_iter if kwargs.get("stream", False) or log_iter is None else b"".join(log_iter) # type: ignore[return-value]
Copy link

Copilot AI Oct 15, 2025

Choose a reason for hiding this comment

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

The type: ignore[return-value] comment masks a type mismatch. The return type should be explicitly defined or the function signature should be updated to accurately reflect all possible return types (Container, Generator, Iterator, bytes, or None).

Copilot uses AI. Check for mistakes.
@jwhonce
Copy link
Member

jwhonce commented Oct 15, 2025

@dklimpel Should the Iterator also be updated to return bytes, as copilot suggests? The ignores are acceptable to me.

@jwhonce jwhonce assigned jwhonce and unassigned vrothberg Oct 15, 2025
@dklimpel
Copy link
Contributor Author

dklimpel commented Oct 15, 2025

@dklimpel Should the Iterator also be updated to return bytes, as copilot suggests? The ignores are acceptable to me.

I would only fix this one issue. For a decision if the iterator is a str or bytes it needs a deep dive.

EDIT:
From a conceptual point of view, it makes sense to have standardized return types. container.logs also returns bytes. I will adjust it.

Signed-off-by: dklimpel <5740567+dklimpel@users.noreply.github.com>
Signed-off-by: dklimpel <5740567+dklimpel@users.noreply.github.com>
@jwhonce
Copy link
Member

jwhonce commented Oct 16, 2025

LGTM

@jwhonce jwhonce requested a review from inknos October 16, 2025 14:47
@inknos
Copy link
Contributor

inknos commented Oct 16, 2025

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 16, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 16, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dklimpel, inknos, jwhonce

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

The pull request process is described 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

@openshift-merge-bot openshift-merge-bot bot merged commit b8f900d into containers:main Oct 16, 2025
22 checks passed
@dklimpel dklimpel deleted the fix_typing_containers_run branch October 16, 2025 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants