Skip to content

Feature/add image import#634

Open
Andre-85 wants to merge 1 commit intocontainers:mainfrom
Andre-85:feature/add_image_import
Open

Feature/add image import#634
Andre-85 wants to merge 1 commit intocontainers:mainfrom
Andre-85:feature/add_image_import

Conversation

@Andre-85
Copy link

@Andre-85 Andre-85 commented Mar 5, 2026

Hi @ALL,
while using podman-py i realized that there is no python-binding for podman import, so i created one by .myself.

I works straight forward:

import podman
c = podman.PodmanClient()
c.images.import_image(<tarfile name>, reference=<like in the podman import command>)

I tested it successfully on Ubuntu 24.04 with podman 4.9.3 and on Ubuntu 26.04 snapshot 3 (snapshot 4 does not work on qemu for some reason) and podman 5.7.0.

What do you think?

Greetings,
André

@Andre-85 Andre-85 force-pushed the feature/add_image_import branch from 0a359b2 to aace063 Compare March 6, 2026 08:19
Copy link
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

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

Please add tests for this. Ideally integration tests, since it doesn’t work on your machine with the latest version of Podman.

@Andre-85
Copy link
Author

Andre-85 commented Mar 6, 2026

Please add tests for this. Ideally integration tests, since it doesn’t work on your machine with the latest version of Podman.

Yes i will do in the next days.

Greetings,
André

"""Import a tarball as an image (equivalent of 'podman import').

Args:
source: Path to a tarball (str) or a file-like object opened in binary mode.
Copy link
Member

Choose a reason for hiding this comment

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

This arg does not exist.

APIError: when service returns an error.
"""
# Check that exactly one of the data or file_path is provided
if not data and not file_path:
Copy link
Member

Choose a reason for hiding this comment

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

I would use is None check.

self.client.images.import_image(b'data', b'file_path')

with self.assertRaises(PodmanError):
self.client.images.import_image(data=b'data', file_path=b'file_path')
Copy link
Member

Choose a reason for hiding this comment

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

Why is file_path a bytes literal?

if file_path:
# Convert to Path if file_path is a string
file_path_object = Path(file_path)
post_data = file_path_object.read_bytes() # Read the tarball file as bytes
Copy link
Member

Choose a reason for hiding this comment

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

I would stream this data instead of reading a multiple GB file into memory.

@Honny1
Copy link
Member

Honny1 commented Mar 9, 2026

Also, please squash your changes into one commit.

@Andre-85
Copy link
Author

Also, please squash your changes into one commit.

Yes, will do and i will apply your suggestions.

@Andre-85 Andre-85 force-pushed the feature/add_image_import branch 2 times, most recently from 71692e1 to eae68f5 Compare March 10, 2026 10:23
@Andre-85 Andre-85 requested a review from Honny1 March 10, 2026 10:52
@Andre-85
Copy link
Author

Also, please squash your changes into one commit.

Yes, will do and i will apply your suggestions.

I applied the requested changes, added extra tests for setting message and changes and squashed all changes in one commit.

Copy link
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

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

ruff (legacy alias)......................................................Failed
- hook id: ruff
- exit code: 1

UP035 `typing.List` is deprecated, use `list` instead
  --> podman/domain/images_manager.py:9:1
   |
 7 | import os
 8 | import urllib.parse
 9 | from typing import Any, Literal, Optional, Union, List
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10 | from collections.abc import Iterator, Mapping, Generator
11 | from pathlib import Path
   |

UP006 Use `list` instead of `List` for type annotation
   --> podman/domain/images_manager.py:175:27
    |
173 |         reference: Optional[str] = None,
174 |         message: Optional[str] = None,
175 |         changes: Optional[List[str]] = None,
    |                           ^^^^
176 |     ) -> "Image":
177 |         """Import a tarball as an image (equivalent of 'podman import').
    |
help: Replace with `list`

F841 Local variable `tar_path` is assigned to but never used
   --> podman/tests/integration/test_images.py:327:13
    |
325 |             # Pack the testfile with the test folder in a tar buffer
326 |             tar_buffer = io.BytesIO()
327 |             tar_path = os.path.join(tmpdir, "test.tar.gz")
    |             ^^^^^^^^
328 |             with tarfile.open(fileobj=tar_buffer, mode="w:gz") as tar:
329 |                 tar.add(base_dir, arcname="test")
    |
help: Remove assignment to unused variable `tar_path`

Found 3 errors.
No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option).

@Andre-85 Andre-85 force-pushed the feature/add_image_import branch 2 times, most recently from 15fa761 to 0cab523 Compare March 10, 2026 20:58
@Andre-85 Andre-85 requested a review from Honny1 March 10, 2026 21:15
@Andre-85
Copy link
Author

ruff (legacy alias)......................................................Failed
- hook id: ruff
- exit code: 1

UP035 `typing.List` is deprecated, use `list` instead
  --> podman/domain/images_manager.py:9:1
   |
 7 | import os
 8 | import urllib.parse
 9 | from typing import Any, Literal, Optional, Union, List
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
10 | from collections.abc import Iterator, Mapping, Generator
11 | from pathlib import Path
   |

UP006 Use `list` instead of `List` for type annotation
   --> podman/domain/images_manager.py:175:27
    |
173 |         reference: Optional[str] = None,
174 |         message: Optional[str] = None,
175 |         changes: Optional[List[str]] = None,
    |                           ^^^^
176 |     ) -> "Image":
177 |         """Import a tarball as an image (equivalent of 'podman import').
    |
help: Replace with `list`

F841 Local variable `tar_path` is assigned to but never used
   --> podman/tests/integration/test_images.py:327:13
    |
325 |             # Pack the testfile with the test folder in a tar buffer
326 |             tar_buffer = io.BytesIO()
327 |             tar_path = os.path.join(tmpdir, "test.tar.gz")
    |             ^^^^^^^^
328 |             with tarfile.open(fileobj=tar_buffer, mode="w:gz") as tar:
329 |                 tar.add(base_dir, arcname="test")
    |
help: Remove assignment to unused variable `tar_path`

Found 3 errors.
No fixes available (2 hidden fixes can be enabled with the `--unsafe-fixes` option).

I fixed the issues.

"""Import a tarball as an image (equivalent of 'podman import').

Args:
file_path: Path to a tarball (str) or a file-like object opened in binary mode.
Copy link
Member

Choose a reason for hiding this comment

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

I think description doesn't match file_path: Optional[os.PathLike] = None.

Copy link
Author

Choose a reason for hiding this comment

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

I fixed it.

response.raise_for_status()

body = response.json()
image_id = body.get("Id") or body.get("id")
Copy link
Member

Choose a reason for hiding this comment

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

What if [Ii]d is missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, this should be handled for failure of getting Id

Copy link
Author

@Andre-85 Andre-85 Mar 11, 2026

Choose a reason for hiding this comment

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

I found out the true field name is "Id" so i will leave the other one out.

I guess i found a similar error handling as requested in the load function:

        response.raise_for_status()  # Catch any errors before proceeding
  
          def _generator(body: dict) -> Generator[Image, None, None]:
              # Iterate and yield images from response body
              for item in body["Names"]:
                  yield self.get(item)
  
          # Pass the response body to the generator
          return _generator(response.json())

But being honest, I don't get what is happening here...

@Honny1
Copy link
Member

Honny1 commented Mar 11, 2026

PTAL @inknos

@inknos inknos self-requested a review March 11, 2026 13:00
Comment on lines +171 to +175
data: Optional[bytes] = None,
file_path: Optional[os.PathLike] = None,
reference: Optional[str] = None,
message: Optional[str] = None,
changes: Optional[builtins.list[str]] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'd like to see only the required args as named, the others being kwargs. It's an arbitrary approach, but the library is designed this way, so I would follow this design pattern.

def import_image(
    self,     
    data: Optional[bytes] = None,
    file_path: Optional[os.PathLike] = None,
    **kwargs,

Kwargs being: reference, message, changes, and url, which is missing. Do you plan to implement it? if not, we'll have to document it with TODO and reference a new issue in the docstring.

Copy link
Author

@Andre-85 Andre-85 Mar 11, 2026

Choose a reason for hiding this comment

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

i fixed the style. reference, message and changes are implemented and tested (in the integration test).

The url parameter i did not implement (yet). Would be a solution with with urllib.request.urlopen(source) as response: ... ok for you?

Copy link
Author

Choose a reason for hiding this comment

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

Or alternate with requests:

  with requests.get('https://httpbin.org/get', stream=True) as r:
    for chunk in r.iter_content(chunk_size=8192):
        ....

Comment on lines +203 to +209
params = {}
if reference:
params["reference"] = reference
if message:
params["message"] = message
if changes:
params["changes"] = changes # requests sends repeated keys as a list
Copy link
Contributor

Choose a reason for hiding this comment

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

Following the comment above, this can be simplified with the same logic of other functions that do kwargs.get()

Copy link
Author

Choose a reason for hiding this comment

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

i fixed it

@Andre-85
Copy link
Author

@Honny1
@inknos

I will do the remaining fixes in the next days. After resolving all i will squeeze them in one commit.

@Andre-85 Andre-85 force-pushed the feature/add_image_import branch 2 times, most recently from f8a46e7 to cb765a8 Compare March 13, 2026 08:40
for importing raw tar files. This corresponds "podman image import <path> [reference]" on the command line

Signed-off-by: Andre Wagner <[email protected]>
@Andre-85 Andre-85 force-pushed the feature/add_image_import branch from 165cfdc to b235fe9 Compare March 15, 2026 20:52
@Andre-85
Copy link
Author

Hi,
I implemented the request changes and I implemented the missing url feature including unit and integration tests.

Greetings,
André

@Andre-85 Andre-85 requested a review from Honny1 March 16, 2026 08:18
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.

3 participants