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

If set mysql configuration in playbook do not override default value … #40183

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@kucuny

kucuny commented May 15, 2018

…only for host, port, socket (#26919)

SUMMARY

Fixed #26919

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

mysql_db

ANSIBLE VERSION
ansible 2.6.0 (prevent-override-login-host-when-already-set-mycnf a3c2e26f66) last updated 2018/05/15 13:44:21 (GMT -400)
  config file = None
  configured module search path = ['/Users/kucuny/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/kucuny/works/osp/ansible/lib/ansible
  executable location = /Users/kucuny/works/osp/ansible/bin/ansible
  python version = 3.6.1 (default, Mar 23 2017, 16:49:06) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.42.1)]
ADDITIONAL INFORMATION
# Test playbook yaml
---
- name: Mysql
  hosts: localhost
  gather_facts: no
  tasks:
    - name: dump from mysql
      mysql_db:
        name: <DATABASE_NAME>
        state: dump
        login_user: root
        login_password: root
        config_file: ~/my.cnf
        target: /tmp/testdump.sql.gz
# Test my.cnf
[client]
port        = 3306
host        = 127.0.0.1
ansible-playbook -v test.yml -i hosts
No config file found; using defaults

PLAY [Mysql] *********************************************************************************************************

TASK [dump from mysql] ***********************************************************************************************
changed: [localhost] => {"changed": true, "db": "<DATABASE_NAME>", "msg": ""}

PLAY RECAP ***********************************************************************************************************
localhost                  : ok=1    changed=1    unreachable=0    failed=0
 ll /tmp/testdump.sql.gz
-rw-r--r--  1 kucuny  wheel  20 May 15 13:41 [1]  /tmp/testdump.sql.gz

@ansibot ansibot removed the needs_revision label May 15, 2018

except ImportError:
import ConfigParser as configparser
else:
import configparser

This comment has been minimized.

@abadger

abadger May 16, 2018

Member

This can be directly imported from six:

from ansible.module_utils.six.moves import configparser

This comment has been minimized.

@kucuny

kucuny May 16, 2018

@abadger I changed my code. Thanks for great review! :)

if cp and cp.has_section('client'):
module.params['login_unix_socket'] = cp.get('client', 'socket', fallback=module.params['login_unix_socket'])
module.params['login_host'] = cp.get('client', 'host', fallback=module.params['login_host'])
module.params['login_port'] = cp.getint('client', 'port', fallback=module.params['login_port'])

This comment has been minimized.

@abadger

abadger May 16, 2018

Member

This looks like it is unfortunately not quite this simple. We want the following fallbacks:

  • If the user specifies a value explicitly in the playbook, use that
  • If the config file has it, then use that.
  • If both of those do not have a value, then use the module's defaults.

This likely needs changes both here and in the mysql modules themselves. The change here will look something like this (repeat for each of the values we're setting):

if cp an cp.has_section('client'):
    if module.params['login_host'] is None:
        module.params['login_host'] = cp.getint('client', 'host', fallback='localhost')
    [....]

Inside of the mysql modules we'll have to change teh module's argument_spec. For instance, mysql_db.py has this:

        argument_spec=dict(
            login_user=dict(default=None),
            login_password=dict(default=None, no_log=True),
            login_host=dict(default="localhost"),
            [...]

We would need to change it to this:

        argument_spec=dict(
            login_user=dict(default=None),
            login_password=dict(default=None, no_log=True),
            login_host=dict(default=None),
            [...]

This comment has been minimized.

@kucuny

kucuny May 17, 2018

@abadger Okay! I remove login_host default value and set 'socket', 'host' when these value is None. Thanks :)

@jborean93 jborean93 removed the needs_triage label May 17, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented May 17, 2018

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

lib/ansible/modules/database/mysql/mysql_db.py:0:0: E324 Value for "default" from the argument_spec (None) for "login_host" does not match the documentation ('localhost')

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented May 17, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/database/mysql/mysql_replication.py:0:0: E324 Value for "default" from the argument_spec ('localhost') for "login_host" does not match the documentation (None)
lib/ansible/modules/database/mysql/mysql_user.py:0:0: E324 Value for "default" from the argument_spec ('localhost') for "login_host" does not match the documentation (None)
lib/ansible/modules/database/mysql/mysql_variables.py:0:0: E324 Value for "default" from the argument_spec ('localhost') for "login_host" does not match the documentation (None)

click here for bot help

@mattclay

This comment has been minimized.

Member

mattclay commented Jun 12, 2018

In addition to the sanity test failures the integration tests are failing:

https://app.shippable.com/github/ansible/ansible/runs/65942/25/tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment