kubevirt_vm: Improve create VM from template

[machacekondra]

SUMMARY

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

Cherry-pick of : #55927

[ansibot]

@machacekondra: Greetings! Thanks for taking the time to open this pullrequest. In order for the community to handle your pullrequest effectively, we need a bit more information.

Here are the items we could not find in your description:

• issue type

Please set the description of this pullrequest with this template:
https://raw.githubusercontent.com/ansible/ansible/devel/.github/PULL_REQUEST_TEMPLATE.md

click here for bot help

[ansibot]

cc @kubevirt @mmazur @pkliczewski
click here for bot help

[pkliczewski]

@machacekondra Please provide some information what is improved in this PR

[mmazur]

Backporting this to 2.8… do we have any tests that verify this works? Can we add some?

[machacekondra]

Yeah, it's backport of #55927. No tests we have to add some, when we are on openshift.

[pkliczewski]

+1

[abadger]

Hi, this needs a changelog fragment in order to be merged.

[abadger]

A note: As long as we're renaming parameters... this whole function could do with better names for its parameters and variables. For instance, base_dict, and merging_dict might be candidates for x and yy.

[abadger]

Also, staticmethods are usually a sign that the method should be a toplevel function instead. In python, every file is its own namespace so you don't have to throw unrelated functions inside of classes like you do in other languages.

[abadger]

Also note.... from the way you're using merge_dicts in your code, it's probably a lot clearer to implement this using dict.update() like this:

def merge_dicts(base_dict, merging_dicts):
    """This function merges a base dictionary with one or more other dictionaries.
    The base dictionary takes precedence when there is a key collision.
    merging_dicts can be a dict or a list or tuple of dicts.  In the latter case, the
    dictionaries at the front of the list have higher precedence over the ones at the end.
    """
    if not merging_dicts:
        merging_dicts = ({},)

    if not isinstance(merging_dicts, Sequence):
        merging_dicts = (merging_dicts,)

    new_dict = {}
    for d in reversed(merging_dicts):
        new_dict.update(d)

    new_dict.update(base_dict)

    return new_dict

Note about this change.. I think that your current code actually has the opposite precedence when using a list of dicts for the second argument. However, that seems like a really confusing API (In the parameters list, the leftmost dict has precedence but within the second parameter, the rightmost dict has precedence) so I did not replicate that here. You could replicate it if you absolutely need to keep backwards compatibility.

[abadger]

This looks good now. Could you open a PR for devel with the changes that aren't there yet and let me know? Then I can merge that and then merge this backport.

[machacekondra]

This looks good now. Could you open a PR for devel with the changes that aren't there yet and let me know? Then I can merge that and then merge this backport.

Thanks! Here it is: #57179

[abadger]

Merged for the 2.8.1 release. Thanks!