-
Notifications
You must be signed in to change notification settings - Fork 130
Feature/add image import #634
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ | |
| import urllib.parse | ||
| from typing import Any, Literal, Optional, Union | ||
| from collections.abc import Iterator, Mapping, Generator | ||
| from contextlib import nullcontext | ||
| from pathlib import Path | ||
| import requests | ||
|
|
||
|
|
@@ -166,6 +167,87 @@ def _generator(body: dict) -> Generator[Image, None, None]: | |
| # Pass the response body to the generator | ||
| return _generator(response.json()) | ||
|
|
||
| def import_image( | ||
| self, | ||
| data: Optional[bytes] = None, | ||
|
Andre-85 marked this conversation as resolved.
|
||
| file_path: Optional[os.PathLike] = None, | ||
| url: Optional[str] = None, | ||
| **kwargs, | ||
| ) -> "Image": | ||
| """Import a tarball as an image (equivalent of 'podman import'). | ||
|
|
||
| Args: | ||
| file_path: Path to the tarball to import. | ||
| data: tarball raw data (bytes) | ||
| url: Url to the tarball to import. | ||
|
|
||
| Keyword Args: | ||
| reference: Optional reference for the new image (e.g. 'myimage:latest'). | ||
| message: Optional commit message. | ||
| changes: Optional list of Dockerfile-style instructions | ||
| (e.g. ['CMD /bin/bash', 'ENV FOO=bar']). | ||
|
|
||
| Returns: | ||
| An Image object for the newly imported image. | ||
|
|
||
| Raises: | ||
| APIError: when service returns an error. | ||
| """ | ||
| # Check that exactly one of the data or file_path is provided | ||
| if sum(x is not None for x in (data, file_path, url)) != 1: | ||
| raise PodmanError( | ||
| "Exactly one parameter should be set from 'data', 'file_path' and 'url' parameters." | ||
| ) | ||
|
|
||
| # Check if url given it is supported | ||
| if url: | ||
| uri = urllib.parse.urlparse(url) | ||
| if uri.scheme not in api.APIClient.supported_schemes: | ||
| raise ValueError( | ||
| f"The scheme '{uri.scheme}' must be one of {api.APIClient.supported_schemes}" | ||
| ) | ||
|
|
||
| # Set the parameters | ||
| params = {} | ||
| if reference := kwargs.get("reference"): | ||
| params["reference"] = reference | ||
| if message := kwargs.get("message"): | ||
| params["message"] = message | ||
| if changes := kwargs.get("changes"): | ||
| params["changes"] = changes # requests sends repeated keys as a list | ||
|
Comment on lines
+211
to
+217
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i fixed it |
||
| if url: | ||
| params["url"] = url | ||
|
|
||
| # Get either from file or from raw data | ||
| post_data_context = None | ||
| if file_path is not None: | ||
| post_data_context = Path(file_path).open("rb") | ||
| elif data is not None: | ||
| post_data_context = io.BytesIO(data) | ||
| elif url is not None: | ||
| post_data_context = nullcontext() | ||
|
|
||
| # Post it | ||
| image_id = None | ||
| with post_data_context as post_data: | ||
| response = self.client.post( | ||
| "/images/import", | ||
| params=params, | ||
| data=post_data, | ||
| headers={"Content-Type": "application/x-tar"}, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this Header correct when the input is a URL?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did some investigation: I assume that podman --remote is using the API correctly. Since importing images and using an alternate podman socket is not allow sametime (some error like "url and r parameter is not allowed sametime" is shown), put a MITM with socket and started the podman system service on a different socket: The result for an empty file loaded from file_path is: For an non-empty file loaded from file_path: For an non-empty file loaded from url: So in all cases the Content-Type is set to text/plain with looks odd, i would say setting in all cases application/x-tar like I did looks more resonable or? What do you think? Greetings,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems odd. The first thing I noticed is an older version of Podman ( |
||
| ) | ||
| response.raise_for_status() | ||
|
|
||
| body = response.json() | ||
| image_id = body.get("Id") | ||
|
|
||
| if image_id is None: | ||
| raise APIError( | ||
| response.url, response=response, explanation="No image id was returned" | ||
| ) | ||
|
|
||
| return self.get(image_id) | ||
|
|
||
| def prune( | ||
| self, | ||
| all: Optional[bool] = False, # pylint: disable=redefined-builtin | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -17,9 +17,11 @@ | |||||
| import io | ||||||
| import os | ||||||
| import json | ||||||
| import http.server | ||||||
| import platform | ||||||
| import tarfile | ||||||
| import tempfile | ||||||
| import threading | ||||||
| import types | ||||||
| import unittest | ||||||
| import random | ||||||
|
|
@@ -292,3 +294,94 @@ def test_scp(self): | |||||
| e.exception.explanation, | ||||||
| r"failed to connect: dial tcp: lookup fake\.ip\.addr.+no such host", | ||||||
| ) | ||||||
|
|
||||||
| def test_import_from_file(self): | ||||||
|
Andre-85 marked this conversation as resolved.
|
||||||
| with tempfile.TemporaryDirectory() as tmpdir: | ||||||
| # Create test folder with test file | ||||||
| base_dir = os.path.join(tmpdir, "test") | ||||||
| os.mkdir(base_dir) | ||||||
| open(os.path.join(base_dir, "foobar"), "w").close() | ||||||
|
|
||||||
| # Pack the testfile with the test folder in a tar | ||||||
| tar_path = os.path.join(tmpdir, "test.tar.gz") | ||||||
| with tarfile.open(tar_path, "w:gz") as tar: | ||||||
| tar.add(base_dir, arcname="test") | ||||||
|
|
||||||
| # Import it | ||||||
| image = self.client.images.import_image(file_path=tar_path, message="test") | ||||||
| self.assertIsInstance(image, Image) | ||||||
| self.assertEqual(image.attrs.get("Comment"), "test") | ||||||
| container = self.client.containers.create(image, command=["."]) | ||||||
|
|
||||||
| # Check the imported image | ||||||
| actual = container.get_archive("./test/foobar") | ||||||
| self.assertEqual(len(actual), 2) | ||||||
| self.assertEqual(actual[1]["linkTarget"], "/test/foobar") | ||||||
|
|
||||||
| # Clean up | ||||||
| container.remove(force=True) | ||||||
| image.remove(force=True) | ||||||
|
|
||||||
| def test_import_from_data(self): | ||||||
| with tempfile.TemporaryDirectory() as tmpdir: | ||||||
| # Create test folder with test file | ||||||
| base_dir = os.path.join(tmpdir, "test") | ||||||
| os.mkdir(base_dir) | ||||||
| open(os.path.join(base_dir, "foobar"), "w").close() | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In all integration tests.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought since you are not using pathlib at all within your integration tests, I should not use it for a single statement and do a mixture of pathlib / os module file handling?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this looks cleaner, but it is not blocking: base_dir = Path(tmpdir) / "test"
base_dir.mkdir(parents=True, exist_ok=True)
(base_dir / "foobar").touch()
# or
(Path(tmpdir) / "test").mkdir(parents=True, exist_ok=True)
(Path(tmpdir) / "test" / "foobar").touch() |
||||||
|
|
||||||
| # Pack the testfile with the test folder in a tar buffer | ||||||
| tar_buffer = io.BytesIO() | ||||||
| with tarfile.open(fileobj=tar_buffer, mode="w:gz") as tar: | ||||||
| tar.add(base_dir, arcname="test") | ||||||
| tar_buffer.seek(0) | ||||||
|
|
||||||
| # Import it | ||||||
| image = self.client.images.import_image(data=tar_buffer.read(), changes=["ENV FOO=bar"]) | ||||||
| self.assertIsInstance(image, Image) | ||||||
| self.assertEqual(image.attrs.get("Config", {}).get("Env"), ["FOO=bar"]) | ||||||
| container = self.client.containers.create(image, command=["."]) | ||||||
|
|
||||||
| # Check the imported image | ||||||
| actual = container.get_archive("./test/foobar") | ||||||
| self.assertEqual(len(actual), 2) | ||||||
| self.assertEqual(actual[1]["linkTarget"], "/test/foobar") | ||||||
|
|
||||||
| # Clean up | ||||||
| container.remove(force=True) | ||||||
| image.remove(force=True) | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't forget about the container.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In all integration tests.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i was not aware that deleting the image the containers are based on is not deleting them automatically. I will fix it |
||||||
|
|
||||||
| def test_import_from_url(self): | ||||||
| with tempfile.TemporaryDirectory() as tmpdir: | ||||||
| # Create test folder with test file | ||||||
| base_dir = os.path.join(tmpdir, "test") | ||||||
| os.mkdir(base_dir) | ||||||
| open(os.path.join(base_dir, "foobar"), "w").close() | ||||||
|
|
||||||
| # Pack the testfile with the test folder in a tar | ||||||
| tar_path = os.path.join(tmpdir, "test.tar.gz") | ||||||
| with tarfile.open(tar_path, "w:gz") as tar: | ||||||
| tar.add(base_dir, arcname="test") | ||||||
|
|
||||||
| # Serve it on a http server | ||||||
| def handler(*a): | ||||||
| return http.server.SimpleHTTPRequestHandler(*a, directory=tmpdir) | ||||||
|
|
||||||
| with http.server.HTTPServer(("", 0), handler) as httpd: | ||||||
| threading.Thread(target=httpd.serve_forever, daemon=True).start() | ||||||
|
|
||||||
| # Import it | ||||||
| image = self.client.images.import_image( | ||||||
| url=f"http://localhost:{httpd.server_port}/test.tar.gz", message="test" | ||||||
| ) | ||||||
| self.assertIsInstance(image, Image) | ||||||
| self.assertEqual(image.attrs.get("Comment"), "test") | ||||||
| container = self.client.containers.create(image, command=["."]) | ||||||
|
|
||||||
| # Check the imported image | ||||||
| actual = container.get_archive("./test/foobar") | ||||||
| self.assertEqual(len(actual), 2) | ||||||
| self.assertEqual(actual[1]["linkTarget"], "/test/foobar") | ||||||
|
|
||||||
| # Clean up | ||||||
| container.remove(force=True) | ||||||
| image.remove(force=True) | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nonblocking: I'm not sure if
podman-pyis a drop-in replacement fordocker-py. Is it? @inknosIf so, we should probably match
docker-py.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I did some comparison to docker-py.
First i saw that docker-py is using a different endpoint, it uses images/create. But from the description (Link: https://docs.docker.com/reference/api/engine/version/v1.54/#tag/Image/operation/ImageCreate) i would say that looks like misuse of this endpoint, because the descriptions are all pointing to oci images and not to raw tar files.
I also did a comparison with command line podman (podman --remote --debug import import.tar). I can see here that command line podman is also using the equivalent of images/import like my patch. In general I would say that for podman-py it is more important to mimic the behavior of command line podman than the behavior of docker-py.
Further I think that their design for the import_image functions have serious problems (Link: https://github.com/docker/docker-py/blob/main/tests/integration/api_image_test.py#L161) finding out for the src arg, what kind of source it is (file/url or raw bytes). So i think the approach with kwargs data, file_path and url is better and fits more to the load function in podman-py which uses already data.
But i think also providing the convenience wrappers import_image_from_url, import_image_from_data and import_image_from_file like docker-py does should be simple to do.
What do you think?
Greetings,
André
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, Podman tries to be a replacement of Docker, so podman-py should be able to replace docker-py. On one hand, I think it doesn't matter if it is called the libpod API or the compat API of Podman if is supported only for full replacement, but it would mean that with podman-py you cannot use Docker. I am not sure which way this compatibility should work.
cc @inknos
So definitely, I think convenience wrappers are a good idea, but for another PR.