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.

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

docker_image: improve/fix handling of image IDs #87

Merged

Conversation

felixfontein
Copy link
Collaborator

@felixfontein felixfontein commented Feb 17, 2021

SUMMARY

This PR allows to use image IDs in more contexts in docker_image. It also makes the code for loading Docker images aware of image IDs, and fixes that part of #86.

It does not allow to load multiple images though. That should be done in a new module.

ISSUE TYPE
  • Feature Pull Request
  • Bugfix Pull Request
COMPONENT NAME

docker_image

@felixfontein felixfontein changed the title [WIP] docker_image: improve/fix handling of image IDs docker_image: improve/fix handling of image IDs Feb 18, 2021
@felixfontein
Copy link
Collaborator Author

ready_for_review

@felixfontein felixfontein changed the title docker_image: improve/fix handling of image IDs [WIP] docker_image: improve/fix handling of image IDs Feb 18, 2021
@felixfontein
Copy link
Collaborator Author

While working on #90, I found a couple of bugs / oversights that are fixed in 5248b41.

@felixfontein felixfontein changed the title [WIP] docker_image: improve/fix handling of image IDs docker_image: improve/fix handling of image IDs Feb 18, 2021
@felixfontein
Copy link
Collaborator Author

Ok, this is now ready.

Copy link
Contributor

@aminvakil aminvakil left a comment

Choose a reason for hiding this comment

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

LGTM, but I would wait for someone else's review with more experience in python instead of myself :)

@@ -551,6 +554,11 @@ def find_image_by_id(self, image_id):
self.log("Find image %s (by ID)" % image_id)
try:
inspection = self.inspect_image(image_id)
except NotFound as exc:
if not accept_not_there:
Copy link

Choose a reason for hiding this comment

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

I always try to avoid variable names with not in them. This line illustrates a bit why: reading it aloud, it goes like "if not accept not there". Maybe accept_missing_image ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 7f0ec90.

@@ -463,6 +480,9 @@ def absent(self):
if not self.check_mode:
try:
self.client.remove_image(name, force=self.force_absent)
except NotFound as dummy:
Copy link

Choose a reason for hiding this comment

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

Since we are not really using it, I'd remove the as dummy part altogether. It does not make the code better at all.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in b3b6da6.

@russoz
Copy link

russoz commented Feb 28, 2021

LGTM other than the couple of minor issues above, but also putting a disclaimer: I have not used the docker modules, so not very confident on what impacts these changes might have.

@felixfontein felixfontein merged commit f2d16da into ansible-collections:main Feb 28, 2021
@felixfontein
Copy link
Collaborator Author

@aminvakil @russoz thanks a lot for reviewing this!

@felixfontein felixfontein deleted the docker_image-load-ids branch February 28, 2021 09:40
felixfontein added a commit to felixfontein/community.general that referenced this pull request Mar 8, 2021
felixfontein added a commit to felixfontein/ansible that referenced this pull request Mar 8, 2021
felixfontein added a commit to ansible-collections/community.general that referenced this pull request Mar 8, 2021
relrod pushed a commit to ansible/ansible that referenced this pull request Mar 8, 2021
clrpackages pushed a commit to clearlinux-pkgs/ansible that referenced this pull request Mar 16, 2021
…2.9.19

Alexander Sowitzki (1):
      [stable-2.9] Bump azure-pipelines-test-container to version 1.8.0 (#73550)

Ashwini Mhatre (1):
      [stable-2.9]:Fix IOSXR integration test (#73607)

Felix Fontein (4):
      Backport of ansible-collections/community.docker@8702713 (#73638)
      Backport of bugfix parts of ansible-collections/community.docker#87 to stable-2.9. (#73817)
      Backport of ansible-collections/community.docker#88 to stable-1. (#73816)
      Backport of ansible-collections/community.crypto#180 to stable-2.9. (#73815)

Gonéri Le Bouder (2):
      ansible-test: yamllint, check the assigment (#73584)
      [ansible-test] attempt to work around podman (#72096) (#73570)

Mark Chappell (2):
      New AWS module mod_defaults - ec2_vpc_endpoint_service_info (#73670)
      New AWS module mod_defaults - iam_saml_federation (#73669)

Matt Martz (2):
      [stable-2.9] Normalize ConfigParser between Python2 and Python3 (#73715) (#73724)
      [stable-2.9] Don't treat host_pinned as lockstep (#73484) (#73513)

Rhys (1):
      Update mongodb replicaset check_compatibility function (#72299)

Rick Elrod (4):
      Update Ansible release version to v2.9.18.post0.
      New release v2.9.19rc1
      Update Ansible release version to v2.9.19rc1.post0.
      New release v2.9.19

Sam Doran (5):
      Update signing key used in incidental_setup_flatpak_remote tests
      Rebalance Windows test groups to avoid timeouts
      Move win_uri to a group that does not reboot the test instance
      [stable-2.9] hostname - add Almalinux support (#73619) (#73649)
      [stable-2.9] Add AlmaLinux to the family of Red Hat-like operating systems (#73541) (#73544)

Zadkiel (1):
      terraform: Remove line that is suppressing output being shown (#66322) (#73803)
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.

None yet

3 participants