Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions podman/domain/images_build.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,9 +63,10 @@ def build(self, **kwargs) -> tuple[Image, Iterator[bytes]]:
isolation (str) – Isolation technology used during build. (ignored)
use_config_proxy (bool) – (ignored)
http_proxy (bool) - Inject http proxy environment variables into container (Podman only)
layers (bool) - Cache intermediate layers during build.
layers (bool) - Cache intermediate layers during build. Default True.
output (str) - specifies if any custom build output is selected for following build.
outputformat (str) - The format of the output image's manifest and configuration data.
Default to "application/vnd.oci.image.manifest.v1+json" (OCI format).
manifest (str) - add the image to the specified manifest list.
Creates manifest list if it does not exist.
secrets (list[str]) - Secret files/envs to expose to the build
Expand Down Expand Up @@ -172,7 +173,7 @@ def _render_params(kwargs) -> dict[str, list[Any]]:
raise PodmanError("Custom encoding not supported when gzip enabled.")

params = {
"dockerfile": kwargs.get("dockerfile"),
"dockerfile": kwargs.get("dockerfile", f".containerfile.{random.getrandbits(160):x}"),
"forcerm": kwargs.get("forcerm"),
"httpproxy": kwargs.get("http_proxy"),
"networkmode": kwargs.get("network_mode"),
Expand All @@ -187,9 +188,11 @@ def _render_params(kwargs) -> dict[str, list[Any]]:
"squash": kwargs.get("squash"),
"t": kwargs.get("tag"),
"target": kwargs.get("target"),
"layers": kwargs.get("layers"),
"layers": kwargs.get("layers", True),
"output": kwargs.get("output"),
"outputformat": kwargs.get("outputformat"),
"outputformat": kwargs.get(
"outputformat", "application/vnd.oci.image.manifest.v1+json"
),
}

if "buildargs" in kwargs:
Expand All @@ -213,8 +216,5 @@ def _render_params(kwargs) -> dict[str, list[Any]]:
if "secrets" in kwargs:
params["secrets"] = json.dumps(kwargs.get("secrets"))

if params["dockerfile"] is None:
params["dockerfile"] = f".containerfile.{random.getrandbits(160):x}"

# Remove any unset parameters
return dict(filter(lambda i: i[1] is not None, params.items()))
17 changes: 5 additions & 12 deletions podman/tests/integration/test_containers.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import io
import random
import tarfile
import tempfile
import unittest

try:
Expand Down Expand Up @@ -86,7 +85,7 @@ def test_container_crud(self):
with self.subTest("Archive /root/unittest"):
self.assertTrue(container.put_archive("/root", data=tarball))

actual, stats = container.get_archive("/root")
actual, _ = container.get_archive("/root")

with io.BytesIO() as fd:
for chunk in actual:
Expand Down Expand Up @@ -184,16 +183,8 @@ def test_container_commit(self):

def test_container_rm_anonymous_volume(self):
with self.subTest("Check anonymous volume is removed"):
container_file = """
FROM alpine
VOLUME myvol
ENV foo=bar
"""
tmp_file = tempfile.mktemp()
file = open(tmp_file, 'w')
file.write(container_file)
file.close()
self.client.images.build(dockerfile=tmp_file, tag="test-img", path=".")
container_file = io.StringIO("\n".join(["FROM alpine", "VOLUME myvol", "ENV foo=bar"]))
test_img, _ = self.client.images.build(fileobj=container_file, tag="test-img", path=".")

# get existing number of containers and volumes
existing_containers = self.client.containers.list(all=True)
Expand All @@ -211,6 +202,8 @@ def test_container_rm_anonymous_volume(self):
self.assertEqual(len(container_list), len(existing_containers))
volume_list = self.client.volumes.list()
self.assertEqual(len(volume_list), len(existing_volumes))
# clean up
self.client.images.remove(test_img)

def test_container_labels(self):
labels = {'label1': 'value1', 'label2': 'value2'}
Expand Down
46 changes: 42 additions & 4 deletions podman/tests/integration/test_images.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@

import io
import os
import json
import platform
import tarfile
import tempfile
import types
import unittest
import random

import podman.tests.integration.base as base
from podman import PodmanClient
Expand Down Expand Up @@ -145,12 +147,48 @@ def test_corrupt_load(self):
self.assertIn("payload does not match", e.exception.explanation)

def test_build(self):
buffer = io.StringIO("""FROM quay.io/libpod/alpine_labels:latest""")

image, stream = self.client.images.build(fileobj=buffer)
buffer = io.StringIO("""FROM scratch""")
image, _ = self.client.images.build(fileobj=buffer)
self.assertIsNotNone(image)
self.assertIsNotNone(image.id)

def test_build_cache(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

more tests are probably needed to ensure that the defaults are passed, and images.build also accepts the other parameters

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are defaults like those above, and then there is dockerfile that is generated randomly... maybe this is something that should be done in a unit test?

Copy link
Contributor

Choose a reason for hiding this comment

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

There are defaults like those above, and then there is dockerfile that is generated randomly... maybe this is something that should be done in a unit test?

I think you can do all through integration tests by inspecting the request that are passed. if you want I can write you some pseudo-code to help understand how I would design it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes please, I’m not familiar with this kind of stuff… how would I inspect what is requested? I’ve seen how it is mocked in unit tests, is it related?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes please, I’m not familiar with this kind of stuff… how would I inspect what is requested? I’ve seen how it is mocked in unit tests, is it related?

Actually my bad, I was looking into many things at the same time and got confused. You are correct, unit tests is the place where you check you function. Also yes, you said it right, you need to mock a request and test that your image build calls the parameters correctly in the default/non-default cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

more tests are probably needed to ensure that the defaults are passed, and images.build also accepts the other parameters

About the other parameters, you mean that, besides the new defaults, there should be a test should that uses all of them and inspects the request? In that case, there is another issue: while experimenting with the parameter-argument conversion (see above), I discovered that _render_params parses the "remote" key, it is not documented, it can not be used alone because _render_params requires either "path" or "fileobj" (and both are meaningless if remote is used), but other than that the build function is able to handle this, because body is initialized with None, and when all the if/elif fail it stays None which is right for remote. But if dockerfile is None, then it is generated randomly in _render_params and this will be yet another issue: "If the URI points to a tarball and the dockerfile parameter is also specified, there must be a file with the corresponding path inside the tarball" from https://docs.podman.io/en/latest/_static/api.html#tag/images/operation/ImageBuildLibpod
I understand this is going quite out of the "cache" issue scope, I apologize in advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a unit test that checks the default parameters for images.build, with it also I corrected the mock URL for two other test cases

"""Check build caching when enabled

Build twice with caching enabled (default), then again with nocache
"""

def look_for_cache(stream) -> bool:
# Search for a line with contents "-> Using cache <image id>"
uses_cache = False
for line in stream:
parsed = json.loads(line)['stream']
if "Using cache" in parsed:
uses_cache = True
break
return uses_cache

label = str(random.getrandbits(32))
buffer = io.StringIO(f"""FROM scratch\nLABEL test={label}""")
image, _ = self.client.images.build(fileobj=buffer)
buffer.seek(0)
cached_image, stream = self.client.images.build(fileobj=buffer)
self.assertTrue(look_for_cache(stream))
self.assertEqual(
cached_image.id,
image.id,
msg="Building twice with cache does not produce the same image id",
)
# Build again with disabled cache
buffer.seek(0)
uncached_image, stream = self.client.images.build(fileobj=buffer, nocache=True)
self.assertFalse(look_for_cache(stream))
self.assertNotEqual(
uncached_image.id,
image.id,
msg="Building twice without cache produces the same image id",
)

def test_build_with_manifest(self):
buffer = io.StringIO("""FROM quay.io/libpod/alpine_labels:latest""")

Expand Down Expand Up @@ -190,7 +228,7 @@ def add_file(name: str, content: str):
# If requesting a custom context, currently must specify the dockerfile name
self.client.images.build(custom_context=True, fileobj=context)

image, stream = self.client.images.build(
image, _ = self.client.images.build(
fileobj=context,
dockerfile="MyDockerfile",
custom_context=True,
Expand Down
70 changes: 52 additions & 18 deletions podman/tests/unit/test_build.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import io
import requests
import json
import unittest

Expand All @@ -16,6 +17,20 @@
from podman.domain.images import Image
from podman.errors import BuildError, DockerException

good_image_id = "032b8b2855fc"
good_stream = [
{"stream": " ---\u003e a9eb17255234"},
{"stream": "Step 1 : VOLUME /data"},
{"stream": " ---\u003e Running in abdc1e6896c6"},
{"stream": " ---\u003e 713bca62012e"},
{"stream": "Removing intermediate container abdc1e6896c6"},
{"stream": "Step 2 : CMD [\"/bin/sh\"]"},
{"stream": " ---\u003e Running in dba30f2a1a7e"},
{"stream": " ---\u003e 032b8b2855fc"},
{"stream": "Removing intermediate container dba30f2a1a7e"},
{"stream": f"{good_image_id}\n"},
]


class TestBuildCase(unittest.TestCase):
"""Test ImagesManager build().
Expand All @@ -41,19 +56,7 @@ def test_build(self, mock_prepare_containerfile, mock_create_tar):
mock_prepare_containerfile.return_value = "Containerfile"
mock_create_tar.return_value = b"This is a mocked tarball."

stream = [
{"stream": " ---\u003e a9eb17255234"},
{"stream": "Step 1 : VOLUME /data"},
{"stream": " ---\u003e Running in abdc1e6896c6"},
{"stream": " ---\u003e 713bca62012e"},
{"stream": "Removing intermediate container abdc1e6896c6"},
{"stream": "Step 2 : CMD [\"/bin/sh\"]"},
{"stream": " ---\u003e Running in dba30f2a1a7e"},
{"stream": " ---\u003e 032b8b2855fc"},
{"stream": "Removing intermediate container dba30f2a1a7e"},
{"stream": "032b8b2855fc\n"},
]

stream = good_stream
buffer = io.StringIO()
for entry in stream:
buffer.write(json.JSONEncoder().encode(entry))
Expand All @@ -72,9 +75,9 @@ def test_build(self, mock_prepare_containerfile, mock_create_tar):
text=buffer.getvalue(),
)
mock.get(
tests.LIBPOD_URL + "/images/032b8b2855fc/json",
tests.LIBPOD_URL + f"/images/{good_image_id}/json",
json={
"Id": "032b8b2855fc",
"Id": good_image_id,
"ParentId": "",
"RepoTags": ["fedora:latest", "fedora:33", "<none>:<none>"],
"RepoDigests": [
Expand Down Expand Up @@ -104,7 +107,7 @@ def test_build(self, mock_prepare_containerfile, mock_create_tar):
secrets=["id=example,src=podman-build-secret123"],
)
self.assertIsInstance(image, Image)
self.assertEqual(image.id, "032b8b2855fc")
self.assertEqual(image.id, good_image_id)
self.assertIsInstance(logs, Iterable)

@patch.object(api, "create_tar")
Expand Down Expand Up @@ -134,16 +137,47 @@ def test_build_logged_error(self, mock_prepare_containerfile, mock_create_tar):

@requests_mock.Mocker()
def test_build_no_context(self, mock):
mock.post(tests.LIBPOD_URL + "/images/build")
mock.post(tests.LIBPOD_URL + "/build")
with self.assertRaises(TypeError):
self.client.images.build()

@requests_mock.Mocker()
def test_build_encoding(self, mock):
mock.post(tests.LIBPOD_URL + "/images/build")
mock.post(tests.LIBPOD_URL + "/build")
with self.assertRaises(DockerException):
self.client.images.build(path="/root", gzip=True, encoding="utf-8")

@patch.object(api, "create_tar")
@patch.object(api, "prepare_containerfile")
def test_build_defaults(self, mock_prepare_containerfile, mock_create_tar):
"""Check the defaults used by images.build"""
mock_prepare_containerfile.return_value = "Containerfile"
mock_create_tar.return_value = b"This is a mocked tarball."

stream = good_stream
buffer = io.StringIO()
for entry in stream:
buffer.write(json.dumps(entry))
buffer.write("\n")

with requests_mock.Mocker() as mock:
query = "?outputformat=" + (
requests.utils.quote("application/vnd.oci.image.manifest.v1+json", safe='')
+ "&layers=True"
)
mock.post(
tests.LIBPOD_URL + "/build" + query,
text=buffer.getvalue(),
)
mock.get(
tests.LIBPOD_URL + f"/images/{good_image_id}/json",
json={
"Id": "unittest",
},
)
img, _ = self.client.images.build(path="/tmp/context_dir")
assert img.id == "unittest"


if __name__ == '__main__':
unittest.main()