Repository: ansible/ansible
Pull request: Pass the top level dictionaries to combine_vars
##### SUMMARY
Fixes #72975

combine_vars uses dict.update() to replace keys

What we're doing now:
```
>>> vars1 = {'test_hash': {'key1': 'value1', 'key2': 'value2'}}
>>> vars2 = {'test_hash': {'key1': 'newvalue'}}
>>> vars1['test_hash'].update(vars2['test_hash'])
>>> vars1
{'test_hash': {'key1': 'newvalue', 'key2': 'value2'}}
```

What this change does:
```
>>> vars1 = {'test_hash': {'key1': 'value1', 'key2': 'value2'}}
>>> vars2 = {'test_hash': {'key1': 'newvalue'}}
>>> vars1.update(vars2)
>>> vars1
{'test_hash': {'key1': 'newvalue'}}
```

##### ISSUE TYPE
- Bugfix Pull Request


Problem statement:
Hashes are merged instead of replace
##### SUMMARY
Inventory merges hashes instead of replacing them. If you have the same hash variable in 2 different file variables will be merged.
hash_behaviour is not configured, the default "replace".

##### ISSUE TYPE
- Bug Report

##### COMPONENT NAME
Inventory plugin

##### ANSIBLE VERSION
```
ansible 2.10.4
  config file = None
  configured module search path = ['/<snipped>/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = <snipped>/lib64/python3.9/site-packages/ansible
  executable location = <snipped>/bin/ansible
  python version = 3.9.0 (default, Oct  6 2020, 00:00:00) [GCC 10.2.1 20200826 (Red Hat 10.2.1-3)]
```
Also effects:
```
ansible 2.10.1
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/<snipped>.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /<snipped>/lib64/python3.6/site-packages/ansible
  executable location = <snipped>/bin/ansible
  python version = 3.6.8 (default, Aug  7 2019, 17:28:10) [GCC 4.8.5 20150623 (Red Hat 4.8.5-39)]
```

##### CONFIGURATION
```
```

##### OS / ENVIRONMENT
Tested on the latest Fedora 33 and CentOs7

##### STEPS TO REPRODUCE
Create the following two inventory file:
Inventory file1:
```yaml
all:
    hosts:
        localhost:
    vars:
        test_hash:
          key1: value1
          key2: value2
```

Inventory file2:
```yaml
all:
    hosts:
        localhost:
    vars:
        test_hash:
          key1: other_value1
```

Then execute the following command:
```bash
$ ansible-inventory -i ./test_inventory1.yml -i ./test_inventory2.yml --list
{
    "_meta": {
        "hostvars": {
            "localhost": {
                "test_hash": {
                    "key1": "other_value1",
                    "key2": "value2"
                }
            }
        }
    },
    "all": {
        "children": [
            "ungrouped"
        ]
    },
    "ungrouped": {
        "hosts": [
            "localhost"
        ]
    }
}
```

##### EXPECTED RESULTS
_"key2": "value2"_ shouldn't be there because the second test_hash variable should replace the first one.
This was the default behavior before this bug.

``` bash
$ ansible-inventory -i ./test_inventory1.yml -i ./test_inventory2.yml --list
{
    "_meta": {
        "hostvars": {
            "localhost": {
                "test_hash": {
                    "key1": "other_value1",
                }
            }
        }
    },
    "all": {
        "children": [
            "ungrouped"
        ]
    },
    "ungrouped": {
        "hosts": [
            "localhost"
        ]
    }
}
```

##### ACTUAL RESULTS
As you can see the hashes are merged instead of replacing.

```bash
$ ansible-inventory -i ./test_inventory1.yml -i ./test_inventory2.yml --list
{
    "_meta": {
        "hostvars": {
            "localhost": {
                "test_hash": {
                    "key1": "other_value1",
                    "key2": "value2"
                }
            }
        }
    },
    "all": {
        "children": [
            "ungrouped"
        ]
    },
    "ungrouped": {
        "hosts": [
            "localhost"
        ]
    }
}
```

Hints from issue discussion:
Files identified in the description:
* [`lib/ansible/plugins/inventory/__init__.py`](https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/inventory/__init__.py)

If these files are incorrect, please update the `component name` section of the description or use the `!component` bot command.

[click here for bot help](https://github.com/ansible/ansibullbot/blob/master/ISSUE_HELP.md)
<!--- boilerplate: components_banner --->
It looks like this started in 48d4d6ec691b2c749054aa5117761f956d762f1c which fixed `merge`. I get the correct values for both `merge` and `replace` by changing that to combine the top level dictionary:
```diff
diff --git a/lib/ansible/inventory/group.py b/lib/ansible/inventory/group.py
index 4d1d6ab0d6..da26f1fad7 100644
--- a/lib/ansible/inventory/group.py
+++ b/lib/ansible/inventory/group.py
@@ -247,7 +247,7 @@ class Group:
             self.set_priority(int(value))
         else:
             if key in self.vars and isinstance(self.vars[key], MutableMapping) and isinstance(value, Mapping):
-                self.vars[key] = combine_vars(self.vars[key], value)
+                self.vars = combine_vars(self.vars, {key: value})
```

Commit message: changelog

Patch to review:
diff --git a/changelogs/fragments/72979-fix-inventory-merge-hash-replace.yaml b/changelogs/fragments/72979-fix-inventory-merge-hash-replace.yaml
new file mode 100644
index 00000000000000..4b7b24720afe78
--- /dev/null
+++ b/changelogs/fragments/72979-fix-inventory-merge-hash-replace.yaml
@@ -0,0 +1,2 @@
+bugfixes:
+  - inventory - pass the vars dictionary to combine_vars instead of an individual key's value (https://github.com/ansible/ansible/issues/72975).
diff --git a/lib/ansible/inventory/group.py b/lib/ansible/inventory/group.py
index d3efb1d8fa3470..9ba10f594f521e 100644
--- a/lib/ansible/inventory/group.py
+++ b/lib/ansible/inventory/group.py
@@ -246,8 +246,8 @@ def set_variable(self, key, value):
         if key == 'ansible_group_priority':
             self.set_priority(int(value))
         else:
-            if key in self.vars and isinstance(self.vars[key], MutableMapping) and isinstance(value, Mapping):
-                self.vars[key] = combine_vars(self.vars[key], value)
+            if key in self.vars:
+                self.vars = combine_vars(self.vars, {key: value})
             else:
                 self.vars[key] = value
 
diff --git a/lib/ansible/inventory/host.py b/lib/ansible/inventory/host.py
index 7ad300790d0401..0cec7b2651483d 100644
--- a/lib/ansible/inventory/host.py
+++ b/lib/ansible/inventory/host.py
@@ -142,8 +142,8 @@ def remove_group(self, group):
         return removed
 
     def set_variable(self, key, value):
-        if key in self.vars and isinstance(self.vars[key], MutableMapping) and isinstance(value, Mapping):
-            self.vars[key] = combine_vars(self.vars[key], value)
+        if key in self.vars:
+            self.vars = combine_vars(self.vars, {key: value})
         else:
             self.vars[key] = value
 


Code review comments:
diff_hunk: @@ -246,8 +246,8 @@ def set_variable(self, key, value):
         if key == 'ansible_group_priority':
             self.set_priority(int(value))
         else:
-            if key in self.vars and isinstance(self.vars[key], MutableMapping) and isinstance(value, Mapping):
-                self.vars[key] = combine_vars(self.vars[key], value)
+            if key in self.vars:
path: lib/ansible/inventory/group.py
text: @user1:
should we really remove the Mapping checks?

@author:
I guess those checks still makes sense for efficiency - I thought at first it would change the behavior of merging hashes that contain lists, but combine_vars always replaces those.