Skip to content
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

Merged
merged 33 commits into from
Mar 6, 2019

Conversation

s-hertel
Copy link
Contributor

@s-hertel 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

@ansibot
Copy link
Contributor

ansibot commented Jan 2, 2019

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. inventory Inventory category needs_triage Needs a first human triage before being processed. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Jan 2, 2019
lib/ansible/vars/manager.py Outdated Show resolved Hide resolved
@ansibot

This comment has been minimized.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Jan 2, 2019
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Jan 2, 2019
@sivel sivel removed needs_triage Needs a first human triage before being processed. labels Jan 3, 2019
@mattclay

This comment has been minimized.

@mattclay mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Jan 9, 2019
@s-hertel
Copy link
Contributor Author

Rebased for porting guide conflict.

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Feb 27, 2019
@s-hertel
Copy link
Contributor Author

@FutureTecSystems It looks like you have some valuable pieces in lib/ansible/plugins/inventory/foreman.py in that PR. I think it might be out of the scope of this pull request. I've tried to limit my changes to only those necessary for getting cache plugins to be functional with inventory plugins, so optimizing foreman may be better as a follow up PR.

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Feb 28, 2019
@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. core_review In order to be merged, this PR must follow the core review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Mar 4, 2019
Copy link
Contributor

@acozine acozine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @s-hertel for creating and updating the docs for this. Last docs changes suggested.

docs/docsite/rst/dev_guide/developing_inventory.rst Outdated Show resolved Hide resolved
docs/docsite/rst/dev_guide/developing_plugins.rst Outdated Show resolved Hide resolved
acozine and others added 2 commits March 6, 2019 09:57
Co-Authored-By: s-hertel <shertel@redhat.com>
Co-Authored-By: s-hertel <shertel@redhat.com>
@s-hertel s-hertel merged commit 9687879 into ansible:devel Mar 6, 2019
@AlanCoding
Copy link
Member

I'm hitting this message

[DEPRECATION WARNING]: InventoryModule should utilize self._cache as a dict 
instead of self.cache. To set the self._cache dictionary, use self._cache[key] 
= value instead of self.cache.set(key, value). To force update the underlying 
cache plugin with the contents of self._cache before parse() is complete, call 
self.set_cache_plugin and it will use the self._cache dictionary to update the 
cache plugin. This feature will be removed in version 2.12. Deprecation 
warnings can be disabled by setting deprecation_warnings=False in ansible.cfg.

with the openstack inventory plugin.

What does this mean? Is this automatically worthy of creating a new issue in your issue queue, since it looks like the message is directed at the maintainers of the inventory plugin itself, and the openstack inventory plugin is a part of Ansible.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affects_2.8 This issue/PR affects Ansible v2.8 bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. docs This issue/PR relates to or includes documentation. docsite This issue/PR relates to the documentation website. inventory Inventory category new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants