CiscoUcs / ucsm-ansible

Ansible Modules for Cisco Ucs Managed Blade and Rack servers.
59 stars 43 forks source link

[Discussion] Standardise change checking logic #95

Open SDBrett opened 6 years ago

SDBrett commented 6 years ago

Maintenance and creation of modules would be simpler if the logic used for detecting if a change has occurred was more consistent. The goal of introducing a standard approach to this is reduce time spent troubleshooting logic issues, reduce false change detection and ease moving between modules maintained by others.

The current approach is to assume that there is a change by using props_match = False and proving otherwise.

With simple managed objects without child items, this works fine, but come quite complex which child items are included.

When a playbook is run, it takes the input provided and compares to the live system to determine if a change is made. This is the opposite to the above mentioned approach. This is assuming nothing has changed and the checks prove otherwise; or in code props_match = True.

This approach makes no real different to managed objects without child items, but makes a large difference with child times, especially multiple child items.

In addition to the change to assuming true and proving false is that a check system only needs to find a single difference, all individual differences are not required to found.

def check_changed():
    props_match = True
    if parent_mo:
        if module.params['state'] == 'absent':
            props_match = False
        else:
            props_match = parent_mo.check_prop_match(**kwargs)
        if props_match:
            for i in child_mo_settings:
                mo_1 = get_child_mo(i)
                props_match = mo_1.check_prop_match(**kwargs)
                if not props_match:
                    break
        if props_match:
            for i in child2_mo_settings:
                mo_2 = get_child_mo(i)
                props_match = mo_2.check_prop_match(**kwargs)
                if not props_match:
                    break
    else:
        props_match = False

    return props_match

The approach above has a compromise between if statement nesting and minimising the number of return statements.

Thoughts, opinions?

dsoper2 commented 6 years ago

Looks good - thanks for suggesting.