-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
Allow subspec defaults to be processed when the parent argument is not supplied #38967
Changes from all commits
5d8c1b6
68fac96
692fcc6
34f7292
50c937a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1975,7 +1975,13 @@ def _handle_options(self, argument_spec=None, params=None): | |
wanted = v.get('type', None) | ||
if wanted == 'dict' or (wanted == 'list' and v.get('elements', '') == 'dict'): | ||
spec = v.get('options', None) | ||
if spec is None or k not in params or params[k] is None: | ||
if v.get('apply_defaults', False): | ||
if spec is not None: | ||
if params.get(k) is None: | ||
params[k] = {} | ||
else: | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you can extract After that, I think lifting
(untested so likely buggy. and "clearer" is of course subjective, your call...) |
||
elif spec is None or k not in params or params[k] is None: | ||
continue | ||
|
||
self._options_context.append(k) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -412,6 +412,19 @@ def test_fallback_in_option(self, mocker, stdin, options_argspec_dict): | |
assert isinstance(am.params['foobar']['baz'], str) | ||
assert am.params['foobar']['baz'] == 'test data' | ||
|
||
@pytest.mark.parametrize('stdin,spec,expected', [ | ||
({}, | ||
{'one': {'type': 'dict', 'apply_defaults': True, 'options': {'two': {'default': True, 'type': 'bool'}}}}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cosmetic: if you move |
||
{'two': True}), | ||
({}, | ||
{'one': {'type': 'dict', 'options': {'two': {'default': True, 'type': 'bool'}}}}, | ||
None), | ||
], indirect=['stdin']) | ||
def test_subspec_not_required_defaults(self, stdin, spec, expected): | ||
# Check that top level not required, processed subspec defaults | ||
am = basic.AnsibleModule(spec) | ||
assert am.params['one'] == expected | ||
|
||
|
||
class TestLoadFileCommonArguments: | ||
@pytest.mark.parametrize('stdin', [{}], indirect=['stdin']) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Does
default
work together withoptions
, for cases where top-level arg is not supplied?Is
apply_defaults
simply shorthand for adefault
of corresponding dict (in your example giving top_level adefault=dict(second_level=True)
)?If that is true, perhaps implementation doesn't need to complicate
_handle_options
further, could derive an "effective default" without even looking at given params.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
Yes, effectively the goal here, is that a user should not have to duplicate default options from the suboptions, to allow this functionality to work.
A future linting changing will ensure a module does not supply both
apply_defaults
anddefault
together on the same argument.