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

Added a Docker Machine dynamic inventory plugin #54946

Merged
merged 40 commits into from
Aug 7, 2019

Conversation

ximon18
Copy link
Contributor

@ximon18 ximon18 commented Apr 6, 2019

SUMMARY

Added a new dynamic inventory plugin to integrate Ansible with Docker Machine, just like the existing Docker Swarm dynamic inventory plugin in Ansible 2.8.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

docker_machine.py

ADDITIONAL INFORMATION

See https://github.com/ximon18/ansible-docker-machine-inventory-plugin for more information.

The intent of this PR is to begin the process of proposing the module for inclusion in Ansible core. However, there are no unit or integration tests yet. The docker_swarm inventory plugin has integration
tests but has some concerning note in its 'aliases' file about disabling docker due to test instability and also I wouldn't know at this point how to get Docker Machine installed on the integration test platform. I did not find any unit tests for the existing docker_swarm inventory plugin to use as a basis for my own module.

Tested with Ansible 2.7.10, Docker Machine 0.16.0, Python 3.6.7 on Ubuntu 18.04.2 LTS.

Thanks,

Ximon

@ansibot
Copy link
Contributor

ansibot commented Apr 6, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud community_review In order to be merged, this PR must follow the community review workflow. docker inventory Inventory category needs_triage Needs a first human triage before being processed. new_plugin This PR includes a new plugin. python3 support:community This issue/PR relates to code supported by the Ansible community. labels Apr 6, 2019
@felixfontein
Copy link
Contributor

Thanks for creating this PR! Unfortunately, it only contains a symlink to a file not included: /home/ximon/src/ansible-docker-machine-inventory-plugin/docker_machine.py Could you please update the PR? :)

@ximon18
Copy link
Contributor Author

ximon18 commented Apr 6, 2019

Thanks for creating this PR! Unfortunately, it only contains a symlink to a file not included: /home/ximon/src/ansible-docker-machine-inventory-plugin/docker_machine.py Could you please update the PR? :)

Apologies for that silly mistake. Corrected.

@ansibot ansibot removed the needs_triage Needs a first human triage before being processed. label Apr 6, 2019
@ansibot
Copy link
Contributor

ansibot commented Apr 6, 2019

The test ansible-test sanity --test pylint [explain] failed with 12 errors:

lib/ansible/plugins/inventory/docker_machine.py:94:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:111:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:123:46: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:124:52: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:130:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:134:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:136:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:146:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:151:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:156:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:159:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:174:22: ansible-format-automatic-specification Format string contains automatic field numbering specification

The test ansible-test sanity --test docs-build [explain] failed with 1 error:

docs/docsite/rst/plugins/inventory/docker_machine.rst:23:0: warning: Unknown target name: "dm".

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/plugins/inventory/docker_machine.py:44:1: W293 blank line contains whitespace
lib/ansible/plugins/inventory/docker_machine.py:79:1: E302 expected 2 blank lines, found 1

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Apr 6, 2019
Copy link
Contributor

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

Just some quick comments for tonight. ansibot also listed some problems from the CI runs.

lib/ansible/plugins/inventory/docker_machine.py Outdated Show resolved Hide resolved
lib/ansible/plugins/inventory/docker_machine.py Outdated Show resolved Hide resolved
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 6, 2019
@felixfontein
Copy link
Contributor

Apologies for that silly mistake. Corrected.

No problem! Thanks again for submitting this.

Please note that Feature Freeze for Ansible 2.8 is coming up very soon (Thursday, April 11), so the chances for this PR making it into Ansible 2.8 aren't very high. I'll try to give you more feedback tomorrow (gotta go sleeping now), but be warned that I don't have any experience with docker machine.

PS: there are more places where you can improve formatting using the syntax from the link. I just didn't want to mark everything now ;)

@ansibot
Copy link
Contributor

ansibot commented Apr 6, 2019

The test ansible-test sanity --test pylint [explain] failed with 12 errors:

lib/ansible/plugins/inventory/docker_machine.py:94:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:111:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:123:46: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:124:52: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:130:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:134:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:136:56: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:146:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:151:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:156:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:159:30: ansible-format-automatic-specification Format string contains automatic field numbering specification
lib/ansible/plugins/inventory/docker_machine.py:174:22: ansible-format-automatic-specification Format string contains automatic field numbering specification

The test ansible-test sanity --test docs-build [explain] failed with 1 error:

docs/docsite/rst/plugins/inventory/docker_machine.rst:23:0: warning: Unknown target name: "dm".

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/plugins/inventory/docker_machine.py:44:1: W293 blank line contains whitespace
lib/ansible/plugins/inventory/docker_machine.py:79:1: E302 expected 2 blank lines, found 1

click here for bot help

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. community_review In order to be merged, this PR must follow the community review workflow. and removed community_review In order to be merged, this PR must follow the community review workflow. needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 6, 2019
@ximon18
Copy link
Contributor Author

ximon18 commented Apr 7, 2019

Please note that Feature Freeze for Ansible 2.8 is coming up very soon (Thursday, April 11), so the chances for this PR making it into Ansible 2.8 aren't very high. I'll try to give you more feedback tomorrow (gotta go sleeping now), but be warned that I don't have any experience with docker machine.

Thx, v appreciated.

FYI this is where I am using Docker Machine and this is where I invoke Ansible with my Docker Machine dynamic inventory plugin, and this is a playbook that references the created variables. Perhaps it helps to see it used.

PS: there are more places where you can improve formatting using the syntax from the link. I just didn't want to mark everything now ;)

I took a look at the Docker Swarm inventory plugin docs and added a few more uses of the link syntax.

@felixfontein
Copy link
Contributor

Yes, a default of true would be good I think. If you don't mind, I can try adding that over the next days.

Mind? That would be awesome.

One more thought: if the user doesn't want the warning because they don't care about the remote machine having a correctly running Docker Daemon, maybe rather than suppress the warning would it be better to make it possible to disable even attempting to fetch those variables (esp. as sometimes that call to DM is quite slow) ?

What do you think about renaming daemon_required (boolean) to daemon_env (choices) with choices require, require-silently, optional, optional-silently, and skip? Then one can select the desired behavior, whether to be silent, and whether not to even try fetching the env variables. I've implemented that in the above commit.

@ansibot
Copy link
Contributor

ansibot commented Jul 10, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/plugins/inventory/docker_machine.py:183:19: undefined-variable Undefined variable 'demon_env'

click here for bot help

@ansibot ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Jul 10, 2019
@ximon18
Copy link
Contributor Author

ximon18 commented Jul 10, 2019

What do you think about renaming daemon_required (boolean) to daemon_env (choices) with choices require, require-silently, optional, optional-silently, and skip? Then one can select the desired behavior, whether to be silent, and whether not to even try fetching the env variables. I've implemented that in the above commit.

Great work, thank you very much @felixfontein!

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Jul 10, 2019
Copy link
Contributor

@s-hertel s-hertel left a comment

Choose a reason for hiding this comment

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

LGTM

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Jul 18, 2019
@felixfontein
Copy link
Contributor

rebuild_merge

@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Aug 7, 2019
@felixfontein felixfontein reopened this Aug 7, 2019
@ansibot ansibot added shipit This PR is ready to be merged by Core automerge This PR was automatically merged by ansibot. and removed community_review In order to be merged, this PR must follow the community review workflow. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Aug 7, 2019
@bcoca bcoca merged commit 2cca917 into ansible:devel Aug 7, 2019
@felixfontein
Copy link
Contributor

@ximon18 thanks for contributing this inventory plugin! And sorry it took so long to get it merged :) It will be included in Ansible 2.9, to be released this fall/winter.

@WojciechowskiPiotr @s-hertel @mattclay @bcoca thanks a lot for your input and for reviewing!

@ximon18
Copy link
Contributor Author

ximon18 commented Aug 8, 2019

Thankyou @bcoca, and in particular thanks to @felixfontein without whom this would have taken much longer, been much less clear how to proceed and would have been a poorer contribution overall!

@ansible ansible locked and limited conversation to collaborators Sep 6, 2019
@ximon18 ximon18 deleted the docker-machine-inventory-plugin branch September 16, 2019 08:13
@ximon18 ximon18 restored the docker-machine-inventory-plugin branch September 16, 2019 08:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 automerge This PR was automatically merged by ansibot. cloud docker inventory Inventory category new_plugin This PR includes a new plugin. python3 shipit This PR is ready to be merged by Core support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants