fixes an issue with dict_merge in network utils (#41107) #49474
Conversation
ganeshrn
force-pushed
the
backport/2.7/41107
branch
from
eada9fa
to
f07e6ab
Compare
| @@ -275,7 +275,10 @@ def dict_merge(base, other): | |||
| if key in other: | |||
| item = other.get(key) | |||
| if item is not None: | |||
| combined[key] = dict_merge(value, other[key]) | |||
| if isinstance(other[key], dict): | |||
| combined[key] = dict_merge(value, other[key]) | |||
There was a problem hiding this comment.
Shouldn't this be Mapping?
from ansible.module_utils.common._collections_compat import Mapping
[...]
if isinstance(other[key], Mapping):How deeply nested are your dicts? If they're too deeply nested they'll run into the recursion limit. We can cross that bridge if you start to get bug reports about it though....
There was a problem hiding this comment.
Will change it to mapping.
How deeply nested are your dicts?
It depends on how nested the data model is for a particular network device and the data model varies vastly from vendor to vendor so can't say exactly how deeply nested dicts can be.
dict_diff function for network use case is mainly written to get diff between the candidate configuration (user input or module arg spec) and the running config retrieved from the device when action override and dict_merge to merge candidate and running config when action is merge.
There was a problem hiding this comment.
Okay. the default recursionlimit in python is 100 levels. (But it might be set lower if someone has optimized their python build for low memory situations). We'll just have to wait and see if we get any bug reports and fix this then.
ansibot
added
affects_2.7
This change address a problem where the dict_merge function would fail due to the value being a nested dict. This will now recursively pass the value back through the dict_merge function. Merge to devel ansible#41107 (cherry picked from commit 2a4be27) Update changelog Fix review comments
ganeshrn
force-pushed
the
backport/2.7/41107
branch
from
f07e6ab
to
e48d77b
Compare
* Based on review comments for PR ansible#49474 change the check for isinstance from `dict` to `Mapping`
ansibot
added
needs_revision
|
Merged for 2.7.5. Thanks! |
* Based on review comments for PR ansible#49474 change the check for isinstance from `dict` to `Mapping`
SUMMARY
This change address a problem where the dict_merge function would fail
due to the value being a nested dict. This will now recursively pass
the value back through the dict_merge function.
(cherry picked from commit 2a4be27)
ISSUE TYPE
COMPONENT NAME
module_utils/network/common/utils.py
ADDITIONAL INFORMATION