New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inventory cache interface #50446

Open
wants to merge 12 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@s-hertel
Copy link
Contributor

s-hertel commented Jan 2, 2019

SUMMARY

There are a number of problems with the current cache interface, the primary ones being that the developer interface is terrible and it only handles file-backed cache plugins.

Some of the interface problems are that the cache was difficult to set up, get() was expected to raise a KeyError, and the cache had to be set a particular way to be reflected in the backing plugin.

This allows self._cache to be used as a dictionary - e.g. if you want a KeyError, use __getattr__ instead of get. It also updates the backing cache plugin with the value of self._cache if it has been modified after parse is called so the developer doesn't need to worry about the discrepancy between the cache object being exposed (a dictionary) and the permanent cache store (CacheModule).

Added tests and developer documentation.

ISSUE TYPE
  • Bugfix Pull Request
  • Feature Pull Request

@s-hertel s-hertel requested review from bcoca , sivel , nitzmahone , abadger and acozine Jan 2, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 2, 2019

@@ -39,7 +39,9 @@
from ansible.module_utils.common._collections_compat import Mapping, MutableMapping, Sequence
from ansible.module_utils.six import iteritems, text_type, string_types
from ansible.plugins.loader import lookup_loader, vars_loader
from ansible.vars.fact_cache import FactCache
# I'll undo because it might break 3rd party plugins (or could use a toggle + deprecation period?), but this works

This comment has been minimized.

@s-hertel

s-hertel Jan 2, 2019

Contributor

DON'T MERGE - just want to run tests on shippable before I remove this. I'm also interested in opinions about deprecating FactCache. :-)

@s-hertel s-hertel force-pushed the s-hertel:inventory_cache_interface branch from 6932d86 to 8de8310 Jan 2, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 2, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/plugins/cache/test_cache.py:76:15: singleton-comparison Comparison to None should be 'expr is None'

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/vars/manager.py:44:1: E265 block comment should start with '# '
test/units/plugins/cache/test_cache.py:76:38: E711 comparison to None should be 'if cond is None:'

click here for bot help

@s-hertel s-hertel force-pushed the s-hertel:inventory_cache_interface branch from 63eef49 to b8a7db4 Jan 2, 2019

@ansibot ansibot added needs_revision and removed core_review labels Jan 2, 2019

@sivel sivel removed needs_triage labels Jan 3, 2019

@mattclay

This comment has been minimized.

Copy link
Member

mattclay commented Jan 9, 2019

@mattclay mattclay added the ci_verified label Jan 9, 2019

@ansibot ansibot removed the ci_verified label Jan 14, 2019

@@ -21,8 +21,6 @@

import os

from collections import defaultdict

This comment has been minimized.

@s-hertel

s-hertel Jan 14, 2019

Contributor

I could remove the changes to this file, it's irrelevant to the feature I've added. I just noticed it was masking the real fact_cache and so the tests weren't failing when I broke it.

@s-hertel s-hertel force-pushed the s-hertel:inventory_cache_interface branch from ae9d96c to e8a99bd Jan 14, 2019

@@ -21,9 +21,10 @@ class FactCache(MutableMapping):

def __init__(self, *args, **kwargs):

self._plugin = cache_loader.get(C.CACHE_PLUGIN)
if not self._plugin:
_plugin = cache_loader.get(C.CACHE_PLUGIN, class_only=True)

This comment has been minimized.

@s-hertel

s-hertel Jan 14, 2019

Contributor

I'm not sure how this was intended to be used and therefore what the right fix is. The alternative solution for this (and this) is https://github.com/ansible/ansible/pull/40105/files#diff-cb51550dcfdbd27a3f224be2de15e2e8R396. Either the config definition loading must happen first to work with config or class_only needs to be used so the definitions are loaded first.

@ansibot ansibot added core_review and removed needs_revision labels Jan 14, 2019

@ansibot ansibot added the stale_ci label Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment