diff --git a/docs/docsite/rst/porting_guides/porting_guide_2.8.rst b/docs/docsite/rst/porting_guides/porting_guide_2.8.rst index 917416d9890..d47e14537f3 100644 --- a/docs/docsite/rst/porting_guides/porting_guide_2.8.rst +++ b/docs/docsite/rst/porting_guides/porting_guide_2.8.rst @@ -63,7 +63,8 @@ Deprecated 2. The ``FactCache.update()`` method has been converted to follow the dict API. It now takes a dictionary as its sole argument and updates itself with the dictionary's items. The previous API where ``update()`` took a key and a value will now issue a deprecation warning and will be - removed in 2.12. + removed in 2.12. If you need the old behaviour switch to ``FactCache.first_order_merge()`` + instead. Modules ======= diff --git a/lib/ansible/vars/fact_cache.py b/lib/ansible/vars/fact_cache.py index 4db8fd5b440..25ffe70e491 100644 --- a/lib/ansible/vars/fact_cache.py +++ b/lib/ansible/vars/fact_cache.py @@ -59,24 +59,40 @@ class FactCache(MutableMapping): self._plugin.flush() def update(self, *args): - """ We override the normal update to ensure we always use the 'setter' method """ + """ + Backwards compat shim + + We thought we needed this to ensure we always called the plugin's set() method but + MutableMapping.update() will call our __setitem__() just fine. It's the calls to update + that we need to be careful of. This contains a bug:: + + fact_cache[host.name].update(facts) + + It retrieves a *copy* of the facts for host.name and then updates the copy. So the changes + aren't persisted. + + Instead we need to do:: + + fact_cache.update({host.name, facts}) + + Which will use FactCache's update() method. + + We currently need this shim for backwards compat because the update() method that we had + implemented took key and value as arguments instead of taking a dict. We can remove the + shim in 2.12 as MutableMapping.update() should do everything that we need. + """ if len(args) == 2: - display.deprecated('Calling FactCache.update(key, value) is deprecated. Use the' - ' normal dict.update() signature of FactCache.update({key: value})' - ' instead.', version='2.12') - host_facts = {args[0]: args[1]} + # Deprecated. Call the new function with this name + display.deprecated('Calling FactCache().update(key, value) is deprecated. Use' + ' FactCache().first_order_merge(key, value) if you want the old' + ' behaviour or use FactCache().update({key: value}) if you want' + ' dict-like behaviour.', version='2.12') + return self.first_order_merge(*args) + elif len(args) == 1: host_facts = args[0] + else: raise TypeError('update expected at most 1 argument, got {0}'.format(len(args))) - for key in host_facts: - try: - host_cache = self._plugin.get(key) - if host_cache: - host_cache.update(host_facts[key]) - else: - host_cache = host_facts[key] - self._plugin.set(key, host_cache) - except KeyError: - self._plugin.set(key, host_facts[key]) + super(FactCache, self).update(host_facts) diff --git a/lib/ansible/vars/manager.py b/lib/ansible/vars/manager.py index c5561b51fe9..afe1b613620 100644 --- a/lib/ansible/vars/manager.py +++ b/lib/ansible/vars/manager.py @@ -629,13 +629,23 @@ class VariableManager: Sets or updates the given facts for a host in the fact cache. ''' - if not isinstance(facts, dict): - raise AnsibleAssertionError("the type of 'facts' to set for host_facts should be a dict but is a %s" % type(facts)) + if not isinstance(facts, Mapping): + raise AnsibleAssertionError("the type of 'facts' to set for host_facts should be a Mapping but is a %s" % type(facts)) try: - self._fact_cache.update({host.name: facts}) + host_cache = self._fact_cache[host.name] except KeyError: - self._fact_cache[host.name] = facts + # We get to set this as new + host_cache = facts + else: + if not isinstance(host_cache, MutableMapping): + raise TypeError('The object retrieved for {0} must be a MutableMapping but was' + ' a {1}'.format(host.name, type(host_cache))) + # Update the existing facts + host_cache.update(facts) + + # Save the facts back to the backing store + self._fact_cache[host.name] = host_cache def set_nonpersistent_facts(self, host, facts): '''