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.

Sign up for GitHub

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

Jump to bottom

docker_image: check output of load_image and react upon #55

Merged
merged 1 commit into from
Dec 27, 2020
Merged

docker_image: check output of load_image and react upon #55

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 3 additions & 0 deletions changelogs/fragments/55-docker_image-loading.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
bugfixes:
- "docker_image - report error when loading a broken archive that contains no image (https://github.com/ansible-collections/community.docker/issues/46, https://github.com/ansible-collections/community.docker/pull/55)."
- "docker_image - report error when the loaded archive does not contain the specified image (https://github.com/ansible-collections/community.docker/issues/41, https://github.com/ansible-collections/community.docker/pull/55)."
38 changes: 33 additions & 5 deletions plugins/modules/docker_image.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@ def build_image(self):
# line = json.loads(line)
self.log(line, pretty_print=True)
if "stream" in line or "status" in line:
build_line = line.get("stream") or line.get("status")
build_line = line.get("stream") or line.get("status") or ''
build_output.append(build_line)

if line.get('error'):
Expand All @@ -665,17 +665,45 @@ def load_image(self):

:return: image dict
'''
# Load image(s) from file
load_output = []
try:
self.log("Opening image %s" % self.load_path)
with open(self.load_path, 'rb') as image_tar:
self.log("Loading image from %s" % self.load_path)
self.client.load_image(image_tar)
for line in self.client.load_image(image_tar):
self.log(line, pretty_print=True)
if "stream" in line or "status" in line:
load_line = line.get("stream") or line.get("status") or ''
load_output.append(load_line)
Copy link

@russoz russoz Dec 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

nitpickers of the world, unite!

I did not dig deeply to see what self.client.load_image() and line really are, but it does seem that the code in the PR is testing for stream and status twice: once explicitly in the if and once more implicitly in the get() (assuming a behaviour similar to dict). If that's indeed the case, it might get clearer as:

                    load_line = line.get("stream") or line.get("status")
                    if load_line:
                        load_output.append(load_line)

Copy link

Choose a reason for hiding this comment

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

If that's not the case, at the very least we should not need the or '' at the end of line 677 because we have tested for stream or status before, so we should never get to that last leg of the or.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Talking about nitpicking: "status" in line does not guarantee that line.get("status") is not falsish, or even None :)

The way the code is currently written, it will produce one string for every line that has status or stream in it, and it is identical to similar code further up (or down?) for building images. (Eventually I want to refactor it, but not yet :) )

except EnvironmentError as exc:
if exc.errno == errno.ENOENT:
self.fail("Error opening image %s - %s" % (self.load_path, str(exc)))
self.fail("Error loading image %s - %s" % (self.name, str(exc)))
self.client.fail("Error opening image %s - %s" % (self.load_path, str(exc)))
self.client.fail("Error loading image %s - %s" % (self.name, str(exc)), stdout='\n'.join(load_output))
except Exception as exc:
self.fail("Error loading image %s - %s" % (self.name, str(exc)))
self.client.fail("Error loading image %s - %s" % (self.name, str(exc)), stdout='\n'.join(load_output))

# Collect loaded images
loaded_images = set()
for line in load_output:
if line.startswith('Loaded image:'):
loaded_images.add(line[len('Loaded image:'):].strip())

if not loaded_images:
self.client.fail("Detected no loaded images. Archive potentially corrupt?", stdout='\n'.join(load_output))

expected_image = '%s:%s' % (self.name, self.tag)
if expected_image not in loaded_images:
self.client.fail(
"The archive did not contain image '%s'. Instead, found %s." % (
expected_image, ', '.join(["'%s'" % image for image in sorted(loaded_images)])),
stdout='\n'.join(load_output))
loaded_images.remove(expected_image)

if loaded_images:
self.client.module.warn(
"The archive contained more images than specified: %s" % (
', '.join(["'%s'" % image for image in sorted(loaded_images)]), ))

return self.client.find_image(self.name, self.tag)

Expand Down
28 changes: 27 additions & 1 deletion tests/integration/targets/docker_image/tasks/tests/options.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,11 @@
source: pull
register: archive_image

- name: Create invalid archive
copy:
dest: "{{ output_dir }}/image-invalid.tar"
content: "this is not a valid image"

- name: remove image
docker_image:
name: "{{ docker_test_image_hello_world }}"
Expand All @@ -209,11 +214,32 @@
source: load
register: load_image_1

- name: load image (wrong name)
docker_image:
name: foo:bar
load_path: "{{ output_dir }}/image.tar"
source: load
register: load_image_2
ignore_errors: true

- name: load image (invalid image)
docker_image:
name: foo:bar
load_path: "{{ output_dir }}/image-invalid.tar"
source: load
register: load_image_3
ignore_errors: true

- assert:
that:
- load_image is changed
- load_image_1 is not changed
- archive_image['image']['Id'] == load_image['image']['Id']
- load_image_1 is not changed
- load_image_2 is failed
- >-
"The archive did not contain image 'foo:bar'. Instead, found '" ~ docker_test_image_hello_world ~ "'." == load_image_2.msg
- load_image_3 is failed
- '"Detected no loaded images. Archive potentially corrupt?" == load_image_3.msg'

####################################################################
## path ############################################################
Expand Down