Set Default values correctly for docker variables #42641
Conversation
ansibot
added
affects_2.7
|
The test |
ansibot
added
the
needs_revision
jborean93
removed
the
needs_triage
mattclay
added
the
ci_verified
ansibot
added
needs_rebase
|
Thanks for pointing this problem out! There is a much better solution, though: This will take care of extracting the value from the environment variable if it is defined and the parameter is not specified. (To see more examples, grep for |
| docker_host=dict(type='str', aliases=['docker_url'], default=DEFAULT_DOCKER_HOST), | ||
| tls_hostname=dict(type='str', default=DEFAULT_TLS_HOSTNAME), | ||
| docker_host=dict(type='str', aliases=['docker_url'], default=DEFAULT_DOCKER_HOST, fallback=(env_fallback, 'DOCKER_HOST')), | ||
| tls_hostname=dict(type='str', default=DEFAULT_TLS_HOSTNAME, fallback=(env_fallback, 'DOCKER_TLS_HOSTNAME')), | ||
| api_version=dict(type='str', aliases=['docker_api_version'], default='auto'), |
There was a problem hiding this comment.
This needs a fallback with DOCKER_API_VERSION
| tls=dict(type='bool', default=DEFAULT_TLS), | ||
| tls_verify=dict(type='bool', default=DEFAULT_TLS_VERIFY), | ||
| ssl_version=dict(type='str', default=DEFAULT_SSL_VERSION, fallback=(env_fallback, 'DOCKER_SSL_VERSION')), | ||
| tls=dict(type='bool', default=DEFAULT_TLS, fallback=(env_fallback, 'DOCKER_TLS_VERIFY')), |
There was a problem hiding this comment.
The fallback env variable is called DOCKER_TLS, not DOCKER_TLS_VERIFY (that's the one below).
|
@denraf this PR contains the following merge commits: Please rebase your branch to remove these commits. |
ansibot
added
merge_commit
|
@denraf this PR contains the following merge commits: Please rebase your branch to remove these commits. |
|
@denraf You need to remove the merge commits. Check the link for instructions. |
|
@denraf this PR contains the following merge commits: Please rebase your branch to remove these commits. |
ansibot
added
needs_ci
| api_version=dict(type='str', aliases=['docker_api_version'], default='auto'), | ||
| docker_host=dict(type='str', aliases=['docker_url'], default=DEFAULT_DOCKER_HOST, fallback=(env_fallback, 'DOCKER_HOST')), | ||
| tls_hostname=dict(type='str', default=DEFAULT_TLS_HOSTNAME, fallback=(env_fallback, 'DOCKER_TLS_HOSTNAME')), | ||
| api_version=dict(type='str', aliases=['docker_api_version'], default='auto', fallback=(env_fallback, 'DOCKER_API_VERSION')), | ||
| timeout=dict(type='int', default=DEFAULT_TIMEOUT_SECONDS), |
There was a problem hiding this comment.
Ah, sorry, I forgot one more: timeout is missing the DOCKER_TIMEOUT fallback.
ansibot
removed
merge_commit
|
@kassiansun Can you take a look here? |
|
shipit |
|
bot_status |
Componentslib/ansible/module_utils/docker_common.py Metadatawaiting_on: ansible |
|
@felixfontein Seems we're not maintainers of |
|
Yep. Well, I guess we need help then :) @resmo Can you take a look? |
|
Hi all, is this a regression? Made by 4117b2d ? |
|
AFAICS, there are more things to "fix" or cleanup. IMHO the source of the issue is in But for a "hotfix" this PR looks good to me. We should merge this and cleanup in 2.7 only. shipit |
|
@resmo Ah, you found the source ;) Yes, it looks like it. I think the best way is to remove the env variable parameter from |
|
bot_status |
Componentslib/ansible/module_utils/docker_common.py Metadatawaiting_on: ansible |
| tls_hostname=dict(type='str', default=DEFAULT_TLS_HOSTNAME), | ||
| api_version=dict(type='str', aliases=['docker_api_version'], default='auto'), | ||
| timeout=dict(type='int', default=DEFAULT_TIMEOUT_SECONDS), | ||
| docker_host=dict(type='str', aliases=['docker_url'], default=DEFAULT_DOCKER_HOST, fallback=(env_fallback, ['DOCKER_HOST'])), |
There was a problem hiding this comment.
Could you please update lib/ansible/utils/module_docs_fragments/docker.py to mention these variables, see lib/ansible/utils/module_docs_fragments/vmware.py to an example
There was a problem hiding this comment.
Yes, that would be a good idea. I guess we can get rid of the corresponding paragraph in the notes: section then.
@denraf Can you also update the documentation (in the docs fragment) for cacert_path, cert_path and key_path? They are a bit more tricky, since there the value of the env variable DOCKER_CERT_PATH is taken and modified.
ansibot
added
the
needs_revision
|
After discussion with @felixfontein I'll merge this and @felixfontein will update this in a follow up PR |
* Set Default values correctly
* Add changelog entry for ansible#16748. * Changelog entry for ansible#33579. * Added changelog for ansible#42380. * Added changelog for ansible#42641. * Adding changelog entry for ansible#42857. * Added changelog for ansible#44808.
SUMMARY
Environment variables were no longer respected as the default is set twice.
ISSUE TYPE
COMPONENT NAME
docker common utils
ANSIBLE VERSION
ansible 2.6.0
config file = None
configured module search path = [u'/root/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
ansible python module location = /usr/lib/python2.7/site-packages/ansible
executable location = /usr/bin/ansible
python version = 2.7.5 (default, Feb 20 2018, 09:19:12) [GCC 4.8.5 20150623 (Red Hat 4.8.5-28)]
ADDITIONAL INFORMATION
env:
export DOCKER_TLS_VERIFY=1
export DOCKER_HOST=tcp://192.168.99.20:2376
export DOCKER_CERT_PATH=~/.docker/certs
export DOCKER_DEFAULT_IP=192.168.99.20
export DOCKER_TLS_HOSTNAME="docker-dev"
Before:
"msg": "Error connecting: Error while fetching server API version: ('Connection aborted.', BadStatusLine('\x15\x03\x01\x00\x02\x02\n',))"
After:
changed: [localhost] => (item={'_ansible_parsed': True, '_ansible_item_result': True, '_ansible_item_label': {'exposed_ports': [u'22/tcp', u'22/udp', u'80/tcp', u'443/tcp'], 'dns_servers': [u'10.86.2.1'], 'name': u'haproxy1', 'image': u'dev-redhat-base:latest', 'hostname': u'haproxy1', 'capabilities': [u'NET_ADMIN'], 'docker_host': u'tcp://192.168.99.20:2376', 'command': u'/sbin/init', 'registry': {'url': u'192.168.99.12:6559'}, 'groups': [u'haproxy'], 'volumes': [u'/sys/fs/cgroup:/sys/fs/cgroup:ro'], 'tmpfs': [u'/tmp', u'/run']}, u'ansible_job_id': u'525370450142.2904', 'failed': False, u'started': 1, 'changed': True, 'item': {'exposed_ports': [u'22/tcp', u'22/udp', u'80/tcp', u'443/tcp'], 'dns_servers': [u'192.168.99.1'], 'name': u'haproxy1', 'image': u'dev-redhat-base:latest', 'hostname': u'haproxy1', 'capabilities': [u'NET_ADMIN'], 'docker_host': u'tcp://192.168.99.20:2376', 'command': u'/sbin/init', 'registry': {'url': u'192.168.99.12:6559'}, 'groups': [u'haproxy'], 'volumes': [u'/sys/fs/cgroup:/sys/fs/cgroup:ro'], 'tmpfs': [u'/tmp', u'/run']}, u'finished': 0, u'results_file': u'/root/.ansible_async/525370450142.2904', '_ansible_ignore_errors': None, '_ansible_no_log': False}) => { }
changed: [localhost] => {
"censored": "the output has been hidden due to the fact that 'no_log: true' was specified for this result",
"changed": true
}
META: ran handlers