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_network][docker_container] Shared code should be moved to docker_common.py #49564

Closed
DBendit opened this issue Dec 5, 2018 · 7 comments
Closed

[docker_network][docker_container] Shared code should be moved to docker_common.py #49564

DBendit opened this issue Dec 5, 2018 · 7 comments
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud docker feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community.

Comments

@DBendit
Copy link
Contributor

DBendit commented Dec 5, 2018

SUMMARY

docker_network.py and docker_container.py share a block of code related to handling minimum docker-py and docker API versions above the minimum versions required by the modules. Seeing as this code is duplicated, it should be consolidated in docker_common.py.

ISSUE TYPE
  • Feature Idea
COMPONENT NAME

docker_network
docker_container
docker_common

ADDITIONAL INFORMATION

This code will need to be modified to support configurable passed-in internal (i.e. ignored) options and option minimum versions. docker_network and docker_container will need to be updated to take advantage of this consolidated functionality as well.

This is part of ansible/community#408 and is referenced in a TODO in #49562
@felixfontein
Copy link
Contributor

Let's do this (and the other issue) once #49562 is merged. Do you want to work on it, or should I do that? (Or anyone else is interested?) Whatever you prefer :)

@ansibot
Copy link
Contributor

ansibot commented Dec 5, 2018

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

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 cloud docker feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community. labels Dec 5, 2018
@DBendit DBendit mentioned this issue Dec 5, 2018
Merged
@DBendit
Copy link
Contributor Author

DBendit commented Dec 9, 2018

I'll pick up this and #49563 when #49644 gets merged in.

@DBendit DBendit mentioned this issue Dec 10, 2018
Merged
StephenSorriaux pushed a commit to StephenSorriaux/ansible that referenced this issue Dec 12, 2018
@DBendit @gundalow
Docker common consolidation (ansible#49707)
b67719b 
* [docker] Consolidating Python Boolean conversion for Docker API (ansible#49563)

* [docker] Consolidating docker option min version checks (ansible#49564)

* [docker] Moving option min version checks out of docker_swarm (ansible#49564)

Also renaming Boolean cleanup function and fixing docker_container minimum
version check for network interfaces.

* Cleanup from PR feedback
@felixfontein
Copy link
Contributor

This has been "fixed" by #49707 as well, right?

@DBendit
Copy link
Contributor Author

DBendit commented Dec 12, 2018

resolved_by_pr 49707

@felixfontein
Copy link
Contributor

Sometimes, all that is needed is a simple:

close_me

;-)

@felixfontein
Copy link
Contributor

(let's hope it actually works...)

@ansibot ansibot closed this as completed Dec 12, 2018
abadger pushed a commit that referenced this issue Jan 7, 2019
@DBendit @abadger
Docker common consolidation (#49707)
a11e0c1 
* [docker] Consolidating Python Boolean conversion for Docker API (#49563)

* [docker] Consolidating docker option min version checks (#49564)

* [docker] Moving option min version checks out of docker_swarm (#49564)

Also renaming Boolean cleanup function and fixing docker_container minimum
version check for network interfaces.

* Cleanup from PR feedback
kbreit pushed a commit to kbreit/ansible that referenced this issue Jan 11, 2019
@DBendit @kbreit
Docker common consolidation (ansible#49707)
2fde157 
* [docker] Consolidating Python Boolean conversion for Docker API (ansible#49563)

* [docker] Consolidating docker option min version checks (ansible#49564)

* [docker] Moving option min version checks out of docker_swarm (ansible#49564)

Also renaming Boolean cleanup function and fixing docker_container minimum
version check for network interfaces.

* Cleanup from PR feedback
@ansible ansible locked and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 cloud docker feature This issue/PR relates to a feature request. module This issue/PR relates to a module. support:community This issue/PR relates to code supported by the Ansible community.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@DBendit @felixfontein @ansibot