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 connection plugin: fix config docs and update to use config system #297

Merged
merged 25 commits into from
Mar 21, 2022

Conversation

bcoca
Copy link
Contributor

@bcoca bcoca commented Feb 10, 2022

wean off play_context which did not have the correct data in all cases

Fixes #307.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

connection/docker.py

 wean off play_context which did not have the correct data in all cases
@github-actions
Copy link

github-actions bot commented Feb 10, 2022

Docs Build 📝

Thank you for contribution!✨

This PR has been merged and your docs changes will be incorporated when they are next published.

plugins/connection/docker.py Outdated Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

Hmm, one thing I'm wondering is whether this actually works with older versions of ansible-core than 2.13 / devel branch.

My main issue here are when values are specified as CLI args and task keywords. The old code should happily use such values as ansible-core somehow placed them in _play_context.

@briantist
Copy link
Contributor

Does this change behavior because of ansible/ansible#73268 ?
As in, does getting the vars from _play_context give the result from processing templating, or does it (like current behavior) give you raw values without jinja processing?

@bcoca
Copy link
Contributor Author

bcoca commented Feb 11, 2022

@briantist unrelated, you never get vars from play_context .. it does UPDATE the task variables with some 'connection vars' but not always correctly, specially in loop or delegation context (things i think we finally fixed in current devel)

lookups are normally in charge of templating their own vars, but config should be doing it for it in this case, so its a different bug, see my last comment in that ticket.

plugins/connection/docker.py Show resolved Hide resolved
plugins/connection/docker.py Show resolved Hide resolved
plugins/connection/docker.py Show resolved Hide resolved
plugins/connection/docker.py Outdated Show resolved Hide resolved
plugins/connection/docker.py Outdated Show resolved Hide resolved
plugins/connection/docker.py Outdated Show resolved Hide resolved
plugins/connection/docker.py Show resolved Hide resolved
@felixfontein
Copy link
Collaborator

(If backported this needs to be done manually, if just to remove the new options.)

Copy link
Collaborator

@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.

Looks good to me. @AlanCoding can you try again (just to make sure I didn't screw something up)? @bcoca can you also take a quick look? Thanks!

@AlanCoding
Copy link

👍 latest version of the plugin is working just fine as well.

@@ -115,6 +115,7 @@ def __init__(self, play_context, new_stdin, *args, **kwargs):
# root.

self._docker_args = []
self._container_user_cache = {}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

do not cache user as it is not guaranteed to be the same for all calls in the task

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean that someone could recreate the container during the playbook/role run?

@felixfontein felixfontein changed the title fix config docs and update to use config system docker connection plugin: fix config docs and update to use config system Mar 18, 2022
@felixfontein felixfontein merged commit 37a3264 into ansible-collections:main Mar 21, 2022
@felixfontein
Copy link
Collaborator

@bcoca thanks a lot for fixing this!
@fao89 @AlanCoding thanks for reviewing/testing!

felixfontein pushed a commit to felixfontein/community.docker that referenced this pull request Mar 21, 2022
…stem (ansible-collections#297)

* fix config docs and update to use config system

 wean off play_context which did not have the correct data in all cases

* moar fixes

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* updated for backwards compat

* badmergeresolution

* makeitworks

* attempt to fix unit test

* mocking it# No more than 50 chars. #### 50 chars is here: #

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* nomock

* remove bad paste

* properly load connection to initialize config

* initizlie docker args

* Fix bugs.

* Call _set_conn_data() when needed.

* Cache result of _get_docker_remote_user() now that it is called multiple times per task.

* Fix unit tests.

* list.clear() is Python 3...

* Add changelog.

* Call _set_conn_data() also in _connect().

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 37a3264)
felixfontein added a commit that referenced this pull request Mar 24, 2022
…e config system (#313)

* docker connection plugin: fix config docs and update to use config system (#297)

* fix config docs and update to use config system

 wean off play_context which did not have the correct data in all cases

* moar fixes

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* updated for backwards compat

* badmergeresolution

* makeitworks

* attempt to fix unit test

* mocking it# No more than 50 chars. #### 50 chars is here: #

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* Update plugins/connection/docker.py

Co-authored-by: Felix Fontein <[email protected]>

* nomock

* remove bad paste

* properly load connection to initialize config

* initizlie docker args

* Fix bugs.

* Call _set_conn_data() when needed.

* Cache result of _get_docker_remote_user() now that it is called multiple times per task.

* Fix unit tests.

* list.clear() is Python 3...

* Add changelog.

* Call _set_conn_data() also in _connect().

Co-authored-by: Felix Fontein <[email protected]>
(cherry picked from commit 37a3264)

* Remove 2.2.0 additions.

* Remove unnecessary fragment, adjust URL.

Co-authored-by: Brian Coca <[email protected]>
@bcoca bcoca deleted the fix_conn_config branch April 1, 2022 13:26
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.

New warning about connection plugin
5 participants