Refactor: Need to handle the error, if get_parent_container_for_device() returns None inside reset_device()

[Shivani-chourasiya]

CvpApi provides a method reset_device()- https://github.com/aristanetworks/cvprac/blob/develop/cvprac/cvp_api.py#L2876, which is used for CVP module - cv_device_v3 (state_factory_reset )and called inside module_utils/device_tools.py reset_device() method here- https://github.com/aristanetworks/ansible-cvp/blob/ebcc2ebee58adc93d47938be219e5f397e6dc1c5/ansible_collections/arista/cvp/plugins/module_utils/device_tools.py#L1724

There are two issues in this -

1. There is a condition to check if device info is having 'parentContainerId' here - https://github.com/aristanetworks/cvprac/blob/develop/cvprac/cvp_api.py#L2895, this condition will always be false and it will hit else part because, we are not getting 'parentContainerId' attribute in device info as it is commented from CVP DeviceElement class in device_tools.py - https://github.com/aristanetworks/ansible-cvp/blob/ebcc2ebee58adc93d47938be219e5f397e6dc1c5/ansible_collections/arista/cvp/plugins/module_utils/device_tools.py#L278

   So, we should remove the check from https://github.com/aristanetworks/cvprac/blob/develop/cvprac/cvp_api.py#L2895

2. Line https://github.com/aristanetworks/cvprac/blob/develop/cvprac/cvp_api.py#L2899 will throw error if get_parent_container_for_device() returns None, need to handle this.

[mharista]

Hi @Shivani-chourasiya

1. Why will parentContainerId always be false? Cvprac should not be making decisions based on a custom device element info being sent to the reset_device() by ansible-cvp. The checks in cvprac are based on a device object returned by the cvprac API. Also cvprac is still maintaining backwards compatibility with old CVP versions where certain device data key/values have changed.

2. I agree with this comment. My first question is are you hitting a case where get_parent_container_for_device() is returning None?

[Shivani-chourasiya]

Hi @Shivani-chourasiya

1. Why will parentContainerId always be false? Cvprac should not be making decisions based on a custom device element info being sent to the reset_device() by ansible-cvp. The checks in cvprac are based on a device object returned by the cvprac API. Also cvprac is still maintaining backwards compatibility with old CVP versions where certain device data key/values have changed.
2. I agree with this comment. My first question is are you hitting a case where get_parent_container_for_device() is returning None?

Hi @mharista,

1. I just checked with ansible-cvp methods, we can keep this condition for backward compatibility with old CVP versions and other case of device object returned by cvprac API.

2. I didn't hit such case, I found this while going through the code for unittests. Please let me know if we can do this as a part of my existing PR, I will remove the changes for 'parentContainerId' from the PR.

[mharista]

Hi @Shivani-chourasiya ,

Sure we can address #2 in your current PR. I think a simple way to handle this would be something like what I've placed below:

        if 'parentContainerId' in device:
            from_id = device['parentContainerId']
        else:
            parent_cont = self.get_parent_container_for_device(device['key'])
            if parent_cont and 'key' in parent_cont:
                from_id = parent_cont['key']
            else:
                from_id = ''

[Shivani-chourasiya]

Hi @Shivani-chourasiya ,

Sure we can address #2 in your current PR. I think a simple way to handle this would be something like what I've placed below:

        if 'parentContainerId' in device:
            from_id = device['parentContainerId']
        else:
            parent_cont = self.get_parent_container_for_device(device['key'])
            if parent_cont and 'key' in parent_cont:
                from_id = parent_cont['key']
            else:
                from_id = ''

Thanks, I have updated the PR.