Fix parameters with aliases not being passed

[gravesm]

Depends-On: ansible/ansible-zuul-jobs#1176

SUMMARY

The recently added _is_an_alias() function should check whether the
current key is in the list of aliases.

Fixes: #91

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

[tadeboro]

I did play a bit with this in xlab-steampunk@5b72fcb and once I wrote that last unit test, I knew things will not be as easy to solve as they looked like. We cannot just drop any aliased parameter because we can drop the only piece of information when doing so.

I did not work any further on this because I do not know why all that argument handling is there and would take me too long to figure things out. And the fact that the turbo code needs three fork calls to do the double-fork daemonization dance did not fill me with confidence ;)

Edit: PR I experimented in is #90

[ansible-zuul]

LGTM!

[goneri]

Closing/opening because I had to manually push a missing commit.

[gravesm]

@goneri I think this one is going to fail until ansible-collections/kubernetes.core#261 gets merged.

[goneri]

@goneri I think this one is going to fail until ansible-collections/kubernetes.core#261 gets merged.

Should I disable ansible-test-molecule-kubernetes-core-with-turbo until we land this PR?

[gravesm]

Yeah, let's do that.

[goneri]

recheck

[tadeboro]

If I understand things correctly, this fix is not OK. What if we only see one of the aliases being set? Test case I came up with when I was playing with this: https://github.com/xlab-steampunk/cloud.common/blob/5b72fcba614754bc759c2b85c86a2573f035a151/tests/unit/module_utils/test_turbo_module.py#L149-L155

I am pretty sure this test will fail since argument processing will throw away the bar value.

[gravesm]

@tadeboro The arg spec in your example doesn't look like a legitimate arg spec. It is declaring bar as an alias to foo and then also declaring it as a param with itself as an alias. In fact, when I try to do this, ansible sanity complains:

ERROR: plugins/modules/k8s_log.py:0:0: parameter-alias-self: Argument 'bar' in argument_spec is specified as its own alias
ERROR: plugins/modules/k8s_log.py:0:0: parameter-documented-multiple-times: Argument 'foo' in argument_spec with aliases 'bar' is documented multiple times, namely as 'bar', 'foo'```

[tadeboro]

@tadeboro The arg spec in your example doesn't look like a legitimate arg spec. It is declaring bar as an alias to foo and then also declaring it as a param with itself as an alias.

That is indeed not a valid argspec, but the prepare_args function does not take a regular argspec as an input. Instead, it takes an "expanded" argspec, produced by the expand_argument_specs_aliases. And those things do look like the test cases I prepared

When you updated the tests in this PR, you actually made them useless because they operate on data structures they will never encounter in the wild.

[gravesm]

I think the issue is that the expand_argument_spec_aliases function is not necessary, unless I'm missing something. By the time it runs, the value for any alias that's been specified in the task will have been copied to the actual param, so any alias can be discarded. @goneri may know more about why it's there.

[tadeboro]

Well, if you pass a regular argspec to the prepare_args, all calls to the _is_an_alias will return back False. So if the extended argument specification is not needed, we can also get rid of the alias check.

As I said before, I ran out of time before I was able to determine why exactly argument manipulation is needed, but my gut was telling me that the init_args call at

cloud.common/plugins/module_utils/turbo/module.py

Line 144 in 7bad786

args = self.init_args()

could be replaced by a simple args = ansible.module_utils.basic._ANSIBLE_ARGS. This would also simplify the server implementation a lot, but I am assuming that I am missing some important pieces of information original author of the code considered when writing the code.

[goneri]

When I did that, the arguments AND their aliases were exposed in _ANSIBLE_ARGS. This was problematical when the value was used as argument_spec within the background process.
This _ANSIBLE_ARGS variable is not a public API and its behavior/content may change between two minor versions of Ansible. Also, there is maybe a better way to implement that.

[tadeboro]

When I did that, the arguments AND their aliases were exposed in _ANSIBLE_ARGS.

The _ANSIBLE_ARGS global contains whatever was passed to module's stdin (or what came in the arguments file). If the arguments and aliases were both present there, this was an end-user error and ansible should throw a tantrum.

This was problematical when the value was used as argument_spec within the background process. This _ANSIBLE_ARGS variable is not a public API and its behavior/content may change between two minor versions of Ansible.

Well, while the _ANSIBLE_ARGS is not part of the public API, I do not expect it to be gone anytime soon because it is described in the docs as the official way of passing arguments to modules in unit tests. Removing it now would break a lot of existing code. And the same goes with the behavior change: since the official docs describe what should be there, I find it highly unlikely that the current behavior will change. Besides, the server code also relies on manipulating the _ANSIBLE_ARGS variable, which means that turbo modules already use that private API.

Also, there is maybe a better way to implement that.

I do not doubt this. But in my opinion, perfection should not stand in the way of progress and getting rid of argument mangling would be one large step forward in paying back some technical debt in the turbo module implementation.