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

Fix for with_ini when used with YAML parameters. #49481

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
7 participants
@holser

holser commented Dec 4, 2018

SUMMARY

Lookup ini is not working with current example in Ansible documentation.
Fix with_ini example and unittest

  • Bugfix Pull Request

COMPONENT NAME

lib/ansible/plugins/lookup/ini.py

ANSIBLE VERSION

  config file = None
  configured module search path = ['/Users/sgolovat/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/lib/python3.7/site-packages/ansible
  executable location = /usr/local/bin/ansible
  python version = 3.7.1 (default, Nov  6 2018, 18:46:03) [Clang 10.0.0 (clang-1000.11.45.5)]

ADDITIONAL INFORMATION

Create a playbook test.yml with the following content:

- name: "test"
  hosts: localhost
  gather_facts: no
  tasks:
    - name: "Test ini lookup"
      debug: var=item
      with_ini:
        - .*
        - section: section1
        - file: test.ini
        - re: true

You will need a file test.ini with the following content:

[section1]
a=1
b=2
[section2]
a=1
b=2
c=3

Launch this playbook (ansible-playbook test.yml).

What you get:

[WARNING]: Unable to find 'ansible.ini' in expected paths.

fatal: [localhost]: FAILED! => {"msg": "Invalid filename: 'None'"}
msg:
Invalid filename: 'None'
        to retry, use: --limit @/home/yannig/tmp/test.retry

Expected result:

ok: [localhost] => (item=1) => {
    "changed": false, 
    "item": "1"
}
ok: [localhost] => (item=2) => {
    "changed": false, 
    "item": "2"
}

Thus this example was removed.

@abadger

This comment has been minimized.

Member

abadger commented Dec 4, 2018

We're thinking the documentation might be at fault here.

@abadger

This comment has been minimized.

Member

abadger commented Dec 4, 2018

Yeah, it looks like the documentation is what's wrong. The params should be passed in the first term. More like this:

- name: "test"
  hosts: localhost
  gather_facts: no
  tasks:
    - name: "Test ini lookup"
      debug: var=item
      with_ini:
        - '.* section=section1 file=test.ini re=True'

@holser holser force-pushed the holser:fix_with_ini branch from 91e2f7d to ee78630 Dec 5, 2018

@holser

This comment has been minimized.

holser commented Dec 5, 2018

Please review

@ansibot ansibot added core_review and removed needs_revision labels Dec 5, 2018

@yprokule

LGTM

@abadger

This comment has been minimized.

Member

abadger commented Dec 5, 2018

The documentation is what needs changing here, not code.

Your example will work like this:

- name: "test"
  hosts: localhost
  gather_facts: no
  tasks:
    - name: "Test ini lookup"
      debug: var=item
      with_ini:
        - .* section=section1 file=test.ini re=true

The documentation should be updated with this form replacing the current format.

This lookup module has existed with its form of parameter parsing since Ansible-2.0. The incorrect documentation wsa only added last year. So changing the argument passing to match the incorrect documentation will break existing playbooks. If we were willing to break playbooks, we would convert the arguments to the new standard for passing arguments to lookup plugins rather than to the incorrect documentation:

    - name: "Test ini lookup"
      debug: var=item
      loop: query('ini', '.*', section='section1', file='test.ini', re=True)

needs_revision

@holser holser force-pushed the holser:fix_with_ini branch from ee78630 to a4e9e2d Dec 5, 2018

@holser

This comment has been minimized.

holser commented Dec 5, 2018

@abadger You are right. I have added your sample to documentation. However, this patch fixes another example

    - name: "Test ini lookup"
      debug: var=item
      with_ini:
        - .*
        - section: section1
        - file: test.ini
        - re: true

which is 100%. Since ansible is flexible and community driven I believe all possible language constructions should work identically. Also this patch fixes unit tests/pep8 styling issues as well as closes this issue #34315

@sivel

This comment has been minimized.

Member

sivel commented Dec 5, 2018

However, this patch fixes another example

We have discussed this amongst the core dev team, and stand by the decision that this should only be a docs change, and we should not implement another syntax.

@holser holser force-pushed the holser:fix_with_ini branch 2 times, most recently from 2911473 to 334ed42 Dec 5, 2018

@holser

This comment has been minimized.

holser commented Dec 6, 2018

@sivel and @abadger Thank you very much! I have updated request. However, to pass CI I needed to fix unit test as pytest didn't work :(

@holser holser force-pushed the holser:fix_with_ini branch 3 times, most recently from f13f20d to 5db0883 Dec 6, 2018

@abadger

This comment has been minimized.

Member

abadger commented Dec 7, 2018

rebuild_merge

This looks fine now. Note for next time: Our linter configurations use a longer line length so it probably isn't necessary to split the lines like that.

@holser

This comment has been minimized.

holser commented Dec 7, 2018

@abadger Thank you very much. Good to know about about linter :D

@holser

This comment has been minimized.

holser commented Dec 10, 2018

rebuild_merge

Fix with_ini example and unittest
* Fix example in ini.py
* Fix unittest in test_ini.py to pass CI as latest ansible returns list in
  different order. To prevent such issues in future results are sorted
* PEP8 E501 styling improvements

Co-Authored-By: Sergii Golovatiuk <sgolovat@redhat.com>

@holser holser force-pushed the holser:fix_with_ini branch from 5db0883 to ed17006 Dec 12, 2018

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