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

Introduce new 'required_by' argument_spec option #28662

Open
wants to merge 2 commits into
base: devel
from

Conversation

@dagwieers
Member

dagwieers commented Aug 25, 2017

SUMMARY

This PR introduces a new required_by argument_spec option which allows you to say "if parameter A is set, parameter B and C are required as well".

  • required_if can only add dependencies if a parameter is set to a specific value, not when it is just defined.
  • required_together has a commutative property, so: "Parameter A and B are required together, if one of them has been defined".

As an example, we need this for the complex options that the xml module provides. One of the issues we often see is that users are not using the correct combination of options, and then are surprised that the module does not perform the requested action(s).

This would be solved by adding the correct dependencies, and mutual exclusives. For us this is important to get this shipped together with the new xml module in Ansible v2.4. (This is related to bugfix #28657)

    module = AnsibleModule(
        argument_spec=dict(
            path=dict(type='path', aliases=['dest', 'file']),
            xmlstring=dict(type='str'),
            xpath=dict(type='str'),
            namespaces=dict(type='dict', default={}),
            state=dict(type='str', default='present', choices=['absent', 'present'], aliases=['ensure']),
            value=dict(type='raw'),
            attribute=dict(type='raw'),
            add_children=dict(type='list'),
            set_children=dict(type='list'),
            count=dict(type='bool', default=False),
            print_match=dict(type='bool', default=False),
            pretty_print=dict(type='bool', default=False),
            content=dict(type='str', choices=['attribute', 'text']),
            input_type=dict(type='str', default='yaml', choices=['xml', 'yaml']),
            backup=dict(type='bool', default=False),
        ),
        supports_check_mode=True,
        required_by=dict(
            add_children=['xpath'],
            attribute=['value', 'xpath'],
            content=['xpath'],
            set_children=['xpath'],
            value=['xpath'],
        ),
        required_if=[
            ['count', True, ['xpath']],
            ['print_match', True, ['xpath']],
        ],
        required_one_of=[
            ['add_children', 'content', 'count', 'pretty_print', 'print_match', 'set_children', 'value'],
            ['path', 'xmlstring'],
        ],
        mutually_exclusive=[
            ['add_children', 'content', 'count', 'print_match','set_children', 'value'],
            ['path', 'xmlstring'],
        ],
    )

PS There is no documentation for these argument_spec options, so I don't know where we should add it. A good reference is here: http://mobygeek.net/blog/2016/02/16/ansible-module-development-parameters/

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

module_utils/basic.py

ANSIBLE VERSION

v2.4

@dagwieers dagwieers changed the title from Introduce new "required_by' param_spec option to Introduce new 'required_by' param_spec option Aug 25, 2017

@dagwieers dagwieers requested review from bcoca and jctanner Aug 25, 2017

@dagwieers dagwieers force-pushed the dagwieers:required_by branch Aug 25, 2017

@dagwieers dagwieers changed the title from Introduce new 'required_by' param_spec option to Introduce new 'required_by' argument_spec option Aug 25, 2017

@dagwieers dagwieers force-pushed the dagwieers:required_by branch 2 times, most recently Aug 25, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Aug 25, 2017

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 26, 2018

cc @skvidal
click here for bot help

@dagwieers dagwieers force-pushed the dagwieers:required_by branch from e4ebf54 to 9e05139 Nov 27, 2018

@dagwieers

This comment has been minimized.

Member

dagwieers commented Nov 27, 2018

I would like to get this reconsidered.

The alternative that was proposed (have module writers hook their own functions in the argspec) will not help in the long run, as we want to document parameter dependencies as part of the module documentation as much as possible.

@dagwieers dagwieers force-pushed the dagwieers:required_by branch 3 times, most recently from 28991c0 to 297aceb Nov 27, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Nov 27, 2018

@dagwieers dagwieers force-pushed the dagwieers:required_by branch from 297aceb to 53119ef Nov 27, 2018

I added unit tests in order to get this moving forward...

@ansibot ansibot added the openstack label Nov 27, 2018

@ansibot

This comment was marked as resolved.

Contributor

ansibot commented Nov 27, 2018

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

test/units/module_utils/basic/test_argument_spec.py:297:10: duplicate-key Duplicate key 'bam4' in dictionary

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

lib/ansible/modules/cloud/openstack/os_subnet.py:249:54: bad-whitespace Exactly one space required after comma                                 ['allocation_pool_end','allocation_pool_start'],                                                       ^

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/cloud/openstack/os_subnet.py:249:33: E126 continuation line over-indented for hanging indent
lib/ansible/modules/cloud/openstack/os_subnet.py:249:55: E231 missing whitespace after ','

click here for bot help

@dagwieers dagwieers force-pushed the dagwieers:required_by branch 2 times, most recently from 609a06f to 2fac1ed Nov 27, 2018

@gundalow

This comment has been minimized.

Contributor

gundalow commented Nov 27, 2018

I think this would be a good addition

Introduce new "required_by' argument_spec option
This PR introduces a new **required_by** argument_spec option which allows you to say *"if parameter A is set, parameter B and C are required as well"*.

- The difference with **required_if** is that it can only add dependencies if a parameter is set to a specific value, not when it is just defined.
- The difference with **required_together** is that it has a commutative property, so: *"Parameter A and B are required together, if one of them has been defined"*.

As an example, we need this for the complex options that the xml module provides. One of the issues we often see is that users are not using the correct combination of options, and then are surprised that the module does not perform the requested action(s).

This would be solved by adding the correct dependencies, and mutual exclusives. For us this is important to get this shipped together with the new xml module in Ansible v2.4. (This is related to bugfix #28657)

```python
    module = AnsibleModule(
        argument_spec=dict(
            path=dict(type='path', aliases=['dest', 'file']),
            xmlstring=dict(type='str'),
            xpath=dict(type='str'),
            namespaces=dict(type='dict', default={}),
            state=dict(type='str', default='present', choices=['absent',
'present'], aliases=['ensure']),
            value=dict(type='raw'),
            attribute=dict(type='raw'),
            add_children=dict(type='list'),
            set_children=dict(type='list'),
            count=dict(type='bool', default=False),
            print_match=dict(type='bool', default=False),
            pretty_print=dict(type='bool', default=False),
            content=dict(type='str', choices=['attribute', 'text']),
            input_type=dict(type='str', default='yaml', choices=['xml',
'yaml']),
            backup=dict(type='bool', default=False),
        ),
        supports_check_mode=True,
        required_by=dict(
            add_children=['xpath'],
            attribute=['value', 'xpath'],
            content=['xpath'],
            set_children=['xpath'],
            value=['xpath'],
        ),
        required_if=[
            ['count', True, ['xpath']],
            ['print_match', True, ['xpath']],
        ],
        required_one_of=[
            ['path', 'xmlstring'],
            ['add_children', 'content', 'count', 'pretty_print', 'print_match', 'set_children', 'value'],
        ],
        mutually_exclusive=[
            ['add_children', 'content', 'count', 'print_match','set_children', 'value'],
            ['path', 'xmlstring'],
        ],
    )
```

@dagwieers dagwieers force-pushed the dagwieers:required_by branch from 2fac1ed to fa3b621 Nov 27, 2018

@jborean93

This comment has been minimized.

Contributor

jborean93 commented Nov 28, 2018

If this does get approved we will need to do the same with https://github.com/ansible/ansible/blob/devel/lib/ansible/module_utils/csharp/Ansible.Basic.cs.

@dagwieers dagwieers force-pushed the dagwieers:required_by branch from fa3b621 to 3915d67 Nov 28, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 8, 2018

@ansibot ansibot added the stale_ci label Dec 8, 2018

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