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_container: improve image finding / change detection #62971

Merged
merged 8 commits into from Oct 4, 2019
Merged

docker_container: improve image finding / change detection #62971

merged 8 commits into from Oct 4, 2019

Conversation

felixfontein
Copy link
Contributor

SUMMARY

Fixes #62953.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

docker_container

@ansibot
Copy link
Contributor

ansibot commented Sep 30, 2019

cc @DBendit @WojciechowskiPiotr @akshay196 @chouseknecht @cove @danihodovic @dariko @dusdanig @joshuaconner @jwitko @kassiansun @softzilla @tbouvet @zfil
click here for bot help

@ansibot ansibot added WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. cloud docker module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. labels Sep 30, 2019
@ansibot ansibot added the test This PR relates to tests. label Oct 1, 2019
@felixfontein felixfontein mentioned this pull request Oct 2, 2019
Closed
@felixfontein felixfontein changed the title [WIP] docker_container: improve image finding / change detection docker_container: improve image finding / change detection Oct 2, 2019
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. has_issue and removed WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. labels Oct 2, 2019
@felixfontein
Copy link
Contributor Author

The PR fixes that in check mode, a locally non-existant image is not recognized as a change reason. If this happens in non-check mode, the module will try to pull the image, and the module's result will always be changed.

(If pull: yes is specified, it will also pull, but only return changed if the image actually changed. In check mode, I left the "don't return changed" behavior from before in this case. I also looked at using client.inspect_distribution(), but that doesn't return the image ID. That can be used to also report a probably correct result in the pull: yesscenario, but since it requires some probing similar to whatfind_image` currently does to identify the correct name to look for, it might have unintended consequences by sending the auth credentials to the wrong destination. I'd rather not add this without first finding out how to make sure that there's precisely one probing call to the correct registry.)

@felixfontein
Copy link
Contributor Author

ready_for_review

Andersson007
Andersson007 reviewed Oct 3, 2019
View reviewed changes
test/integration/targets/docker_container/tasks/tests/image-ids.yml
force_absent: yes
state: absent

- name: Create container with alpine image via name (check mode, will pull, same image)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would do this test one more time after create_6 to be sure that changed will be false. And also create_6 after that again (so twice in actual mode for one image), just in case, and also check that changed will be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's not necessary, idempotence for started and created containers is already tested in start-stop.yml.

Andersson007
Andersson007 approved these changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

shipit

@felixfontein felixfontein merged commit 41eafc2 into ansible:devel Oct 4, 2019
@felixfontein
Copy link
Contributor Author

@Andersson007 thanks for reviewing this!

@felixfontein felixfontein deleted the docker_container-fix-image-change-detection branch October 4, 2019 19:50
@Andersson007
Copy link
Contributor

you're always welcome!

felixfontein added a commit to felixfontein/ansible that referenced this pull request Oct 4, 2019
@felixfontein
docker_container: improve image finding / change detection (ansible#6…
c53b117 
…2971)

* Improve image finding / change detection.

* Checked wrong object.

* Improve behavior. (Let docker daemon sort this out.)

* Add changelog.

* Add simple test.

* Fix image name.

* Use new docker_image params.

* Rewrite.

(cherry picked from commit 41eafc2)
felixfontein added a commit to felixfontein/ansible that referenced this pull request Oct 4, 2019
@felixfontein
docker_container: improve image finding / change detection (ansible#6…
110c883 
…2971)

* Improve image finding / change detection.

* Checked wrong object.

* Improve behavior. (Let docker daemon sort this out.)

* Add changelog.

* Add simple test.

* Fix image name.

* Use new docker_image params.

* Rewrite.

(cherry picked from commit 41eafc2)
This was referenced Oct 4, 2019
Merged
Merged
@sivel sivel removed the needs_triage Needs a first human triage before being processed. label Oct 8, 2019
abadger pushed a commit that referenced this pull request Oct 10, 2019
@felixfontein @abadger
docker_container: improve image finding / change detection (#62971)
317086b 
* Improve image finding / change detection.

* Checked wrong object.

* Improve behavior. (Let docker daemon sort this out.)

* Add changelog.

* Add simple test.

* Fix image name.

* Use new docker_image params.

* Rewrite.

(cherry picked from commit 41eafc2)
abadger pushed a commit that referenced this pull request Oct 10, 2019
@felixfontein @abadger
docker_container: improve image finding / change detection (#62971)
4b2992c 
* Improve image finding / change detection.

* Checked wrong object.

* Improve behavior. (Let docker daemon sort this out.)

* Add changelog.

* Add simple test.

* Fix image name.

* Use new docker_image params.

* Rewrite.

(cherry picked from commit 41eafc2)
abadger pushed a commit that referenced this pull request Oct 12, 2019
@felixfontein @abadger
docker_container: improve image finding / change detection (#62971)
6c757c0 
* Improve image finding / change detection.

* Checked wrong object.

* Improve behavior. (Let docker daemon sort this out.)

* Add changelog.

* Add simple test.

* Fix image name.

* Use new docker_image params.

* Rewrite.

(cherry picked from commit 41eafc2)
abadger pushed a commit that referenced this pull request Nov 1, 2019
@felixfontein @abadger
docker_container: improve image finding / change detection (#62971)
9e211c6 
* Improve image finding / change detection.

* Checked wrong object.

* Improve behavior. (Let docker daemon sort this out.)

* Add changelog.

* Add simple test.

* Fix image name.

* Use new docker_image params.

* Rewrite.

(cherry picked from commit 41eafc2)
@ansible ansible locked and limited conversation to collaborators Nov 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Andersson007 Andersson007 Andersson007 approved these changes

Assignees
No one assigned
Labels
affects_2.10 This issue/PR affects Ansible v2.10 bug This issue/PR relates to a bug. cloud community_review In order to be merged, this PR must follow the community review workflow. docker has_issue module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

docker_container: image change not recognized if new image is not known to docker daemon
4 participants
@felixfontein @ansibot @Andersson007 @sivel