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

[pytest] Improve 'cached' decorator #3112

Merged
merged 2 commits into from
Mar 12, 2021
Merged

Conversation

lolyu
Copy link
Contributor

@lolyu lolyu commented Mar 9, 2021

Description of PR

Summary:
Fixes # (issue)

Type of change

  • Bug fix
  • Testbed and Framework(new/improvement)
  • Test case(new/improvement)

Approach

What is the motivation for this PR?

  1. The original cached decorator has limitations that it only supports being used for methods of class AnsibleBaseHost and its derivatives.
  2. FactsCache returns None in either the FactsCache fails to find cached facts or the decorated function returns None.
  3. utils functions in tests/common/utilities that try to retrieve host variables share a similar call pattern that the arguments all have hostname defined and have the same logic to check the inventory in cache reading or add the inventory in cache writing.

How did you do it?

  1. Add a zone_getter argument to the cached decorator to let it receives a customizable function to get the zone.
  2. When FactsCache fails to find cached facts, let it returns FactsCache.NOTEXIST instead of None(None is a valid return from the decorated function)
  3. Add arguments after_write and before_read to the cached decorator to let it have extra functionalities to process the facts after the cache reading/before the cache writing.

How did you verify/test it?

(Pdb) utils.get_group_visible_vars(inv_files, "server_17").keys()
[u'ansible_become_password', u'eos_login', u'skip_ceos_image_downloading', u'ceos_image', 'ansible_check_mode', u'vm_mgmt_gw', u'sonic_default_passwords', u'telemetry_certs', 'ansible_diff_mode', 'ansible_forks', u'eos_root_user', 'playbook_dir', u'eos_default_password', u'vm_console_base', 'ansible_playbook_python', u'mux_simulator_port', u'external_port', 'ansible_facts', u'eos_root_password', 'ansible_verbosity', 'inventory_hostname', u'k8s_master_login', u'secret_group_vars', 'omit', u'host_var_file', u'topologies', u'str2', u'ceos_image_filename', u'memory', u'mgmt_bridge', u'proxy_env', u'eos_default_login', u'ceos_image_orig', u'max_fp_num', u'ptf_bp_ipv6', 'inventory_file', u'switch_login', u'secret_host_vars', u'mgmt_prefixlen', u'skip_image_downloading', u'secret_vars', 'group_names', u'corefile_uploader', u'lab', u'mgmt_gw', u'ansible_user', 'groups', u'hwsku_map', u'restapi_certs', u'ansible_host', u'sonic_image_filename', u'sonic_login', 'ansible_inventory_sources', u'vm_images_url', u'cd_image_filename', u'eos_password', u'root_path', u'example_ixia', 'inventory_dir', u'ptf_bp_ip', u'sonic_password', u'hdd_image_filename', 'ansible_version', u'str', 'inventory_hostname_short', u'supported_vm_types', u'k8s_master_password', u'ansible_password']
(Pdb) utils.get_group_visible_vars(inv_files, "server_17")["mux_simulator_port"]
8080
(Pdb) utils.get_host_vars(inv_files, "str2-7050cx3-acs-09")
{u'pdu_host': u'pdu-119', 'inventory_file': u'/data/repo/cache/refact/sonic-mgmt/ansible/str2', u'ansible_host': u'10.3.147.20', u'hwsku': u'Arista-7050CX3-32S-D48C8', 'inventory_dir': u'/data/repo/cache/refact/sonic-mgmt/ansible'}

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

@lolyu lolyu changed the title Refact cache [pytest] Improve 'cached' decorator Mar 9, 2021
@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2021

This pull request introduces 2 alerts when merging a7aeaf09311c7d51efbc1d8213931c16981ae541 into b4b6d21 - view on LGTM.com

new alerts:

  • 2 for Unused import

@lolyu lolyu force-pushed the refact_cache branch 3 times, most recently from 6d21d9e to bf14562 Compare March 9, 2021 14:08
@lolyu lolyu marked this pull request as ready for review March 9, 2021 14:47
@lolyu lolyu requested a review from a team as a code owner March 9, 2021 14:47
@lolyu
Copy link
Contributor Author

lolyu commented Mar 10, 2021

/AZP run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@bingwang-ms
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lolyu
Copy link
Contributor Author

lolyu commented Mar 10, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lolyu
Copy link
Contributor Author

lolyu commented Mar 10, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lolyu
Copy link
Contributor Author

lolyu commented Mar 11, 2021

/AZP run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lolyu lolyu merged commit d932004 into sonic-net:master Mar 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants