search

ansible-collections / community.hashi_vault

Ansible collection for managing and working with HashiCorp Vault.
https://docs.ansible.com/ansible/devel/collections/community/hashi_vault/index.html
GNU General Public License v3.0
80 stars 58 forks source link

update hvac error handling (Open todo) #391

Open mathijswesterhof opened 11 months ago

mathijswesterhof commented 11 months ago
SUMMARY

This pull request addresses the hashi_vault_common module. In specific, the handling of the HVAC import. In the current situation the import is tried and if failed the exception is handled by setting HAS_HVAC to false. this state is later not used to signal the missing lib to the user. This pull request fixes this by raising an HVAC specific error which in turn is picked up by the modules that use the common module. The reason for raising the custom exception is that the module and plugin utils both make use of this class and both have their own error handling service (being; plugins via AnsibleError and modules via the fail handler of the AnsibleModule class)

Note that

I'm not sure if I can confidently remove all the HVAC import try-catch blocks, but based on this change it is a lot easier to write an adapter that can be used for all current modules and future modules significantly reducing duplicated code.

ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION

Implementing the following change will have a positive effect in replcated code, some examples below for some widely used modules: vault_login afbeelding

vault_list afbeelding

before

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: ModuleNotFoundError: No module named 'hvac'
fatal: [192.x.x.x]: FAILED! => {"changed": false, "msg": "Failed to import the required Python library (hvac) on ansible-box's Python /usr/bin/python3. Please read module documentation and install in the appropriate location. If the required library is installed, but Ansible is using the wrong Python interpreter, please consult the documentation on ansible_python_interpreter"}

after

An exception occurred during task execution. To see the full traceback, use -vvv. The error was: No module named 'hvac'
fatal: [192.X.X.X]: FAILED! => { "changed": false, "msg": "Failed to import the required Python library (hvac) on ansible-box's Python /usr/bin/python3. Please read module documentation and install in the appropriate location. If the required library is installed, but Ansible is using the wrong Python interpreter, please consult the documentation on ansible_python_interpreter"}
mathijswesterhof commented 11 months ago

Small note on this pr, I have no clue how to fully and properly run the pytest tests. I'm constantly getting the error that there's a relative import that's missing even though it's clearly there.

     from .compat import mock
E   ImportError: attempted relative import with no known parent package

I'm probably doing something simply stupid but if anyone can get me up to speed with that, that'll be amazing cause I want to properly test everything, especially with the new modules being build.

just for debugging purposes, I'm currently running pytest -c tests/config.yml ./tests in the root folder of the project

briantist commented 11 months ago

Small note on this pr, I have no clue how to fully and properly run the pytest tests. I'm constantly getting the error that there's a relative import that's missing even though it's clearly there.

     from .compat import mock
E   ImportError: attempted relative import with no known parent package

I'm probably doing something simply stupid but if anyone can get me up to speed with that, that'll be amazing cause I want to properly test everything, especially with the new modules being build.

just for debugging purposes, I'm currently running pytest -c tests/config.yml ./tests in the root folder of the project

Hi @mathijswesterhof , welcome and thank you for putting up this PR.

In ansible collection, we don't really run pytest directly, but rather use ansible-test to invoke them. I highly recommend using that tool's container functionality (which is usually used with pre-built containers). The unit and sanity tests are the easiest ones to run locally on your machine.

The first step is to ensure that your checkout directory structure is correct. Your repository should be checked out in this structure: <parent>/ansible_collections/community/hashi_vault

This structure is important!

Then, from the root of the collection:

ansible-test sanity --docker default
ansible-test units --docker default

Units will run their tests across all supported python versions (within the container). For faster local testing you can choose a single python version like so:

ansible-test units --docker default --python 3.10

For more detailed information, please see our Contributor guide.

Let me know if you still need assistance running tests locally, or if the documentation there can be improved.

I will have to look over this PR more deeply when I have some more time. I'm traveling now so I will be slower to respond and review for the next several weeks, but I will at least try to approve the CI for new commits while I'm away. I strongly recommend getting set up to run the tests locally so that you can get much faster feedback on your changes.

Thank you!

codecov[bot] commented 6 months ago

Codecov Report

Attention: Patch coverage is 98.36066% with 2 lines in your changes missing coverage. Please review.

Project coverage is 99.15%. Comparing base (a5c15bf) to head (54ca1c6). Report is 1 commits behind head on main.

Files Patch % Lines
plugins/module_utils/_hashi_vault_module.py 50.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #391 +/- ## ========================================== - Coverage 99.20% 99.15% -0.06% ========================================== Files 109 109 Lines 6201 6047 -154 Branches 1184 1149 -35 ========================================== - Hits 6152 5996 -156 - Misses 39 41 +2 Partials 10 10 ``` | [Flag](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | Coverage Δ | | |---|---|---| | [env_docker-default](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `99.15% <98.36%> (-0.06%)` | :arrow_down: | | [integration](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `82.56% <69.44%> (+1.44%)` | :arrow_up: | | [sanity](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `39.57% <9.72%> (-1.55%)` | :arrow_down: | | [target_auth_approle](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `89.47% <ø> (ø)` | | | [target_auth_aws_iam](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `50.00% <ø> (ø)` | | | [target_auth_azure](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `53.84% <ø> (ø)` | | | [target_auth_cert](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `86.36% <ø> (ø)` | | | [target_auth_jwt](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `91.30% <ø> (ø)` | | | [target_auth_ldap](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `89.47% <ø> (ø)` | | | [target_auth_none](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `100.00% <ø> (ø)` | | | [target_auth_token](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `71.42% <ø> (ø)` | | | [target_auth_userpass](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `85.71% <ø> (ø)` | | | [target_connection_options](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `74.76% <ø> (ø)` | | | [target_controller](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `83.25% <77.19%> (-0.15%)` | :arrow_down: | | [target_filter_vault_login_token](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `77.77% <ø> (ø)` | | | [target_import](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `39.57% <9.72%> (-1.55%)` | :arrow_down: | | [target_lookup_hashi_vault](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `81.33% <ø> (ø)` | | | [target_lookup_vault_ansible_settings](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.94% <29.72%> (-1.07%)` | :arrow_down: | | [target_lookup_vault_kv1_get](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `100.00% <100.00%> (+8.69%)` | :arrow_up: | | [target_lookup_vault_kv2_get](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `100.00% <100.00%> (+8.88%)` | :arrow_up: | | [target_lookup_vault_list](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `90.00% <ø> (ø)` | | | [target_lookup_vault_login](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `88.57% <ø> (ø)` | | | [target_lookup_vault_read](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `90.00% <ø> (ø)` | | | [target_lookup_vault_token_create](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `79.24% <ø> (ø)` | | | [target_lookup_vault_write](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.82% <36.66%> (-1.14%)` | :arrow_down: | | [target_module_utils](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `96.32% <82.75%> (-0.42%)` | :arrow_down: | | [target_module_vault_database_connection_configure](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.21% <37.93%> (-1.19%)` | :arrow_down: | | [target_module_vault_database_connection_delete](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.02% <37.93%> (-1.20%)` | :arrow_down: | | [target_module_vault_database_connection_read](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.95% <37.93%> (-1.20%)` | :arrow_down: | | [target_module_vault_database_connection_reset](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.02% <37.93%> (-1.20%)` | :arrow_down: | | [target_module_vault_database_connections_list](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.45% <37.93%> (-1.18%)` | :arrow_down: | | [target_module_vault_database_role_create](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.45% <37.93%> (-1.18%)` | :arrow_down: | | [target_module_vault_database_role_delete](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.02% <37.93%> (-1.20%)` | :arrow_down: | | [target_module_vault_database_role_read](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.95% <37.93%> (-1.20%)` | :arrow_down: | | [target_module_vault_database_roles_list](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.66% <60.00%> (+0.03%)` | :arrow_up: | | [target_module_vault_database_rotate_root_creds](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.84% <37.93%> (-1.19%)` | :arrow_down: | | [target_module_vault_database_static_role_create](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.21% <37.93%> (-1.19%)` | :arrow_down: | | [target_module_vault_database_static_role_get_creds](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.95% <37.93%> (-1.20%)` | :arrow_down: | | [target_module_vault_database_static_role_read](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.95% <37.93%> (-1.20%)` | :arrow_down: | | [target_module_vault_database_static_role_rotate_creds](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.84% <37.93%> (-1.19%)` | :arrow_down: | | [target_module_vault_database_static_roles_list](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `54.45% <37.93%> (-1.18%)` | :arrow_down: | | [target_module_vault_kv1_get](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `97.43% <100.00%> (+9.93%)` | :arrow_up: | | [target_module_vault_kv2_delete](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.98% <45.16%> (-0.88%)` | :arrow_down: | | [target_module_vault_kv2_get](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `97.36% <100.00%> (+10.13%)` | :arrow_up: | | [target_module_vault_kv2_write](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `56.40% <47.36%> (-0.86%)` | :arrow_down: | | [target_module_vault_list](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `96.96% <100.00%> (+11.25%)` | :arrow_up: | | [target_module_vault_login](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `93.93% <ø> (+10.21%)` | :arrow_up: | | [target_module_vault_pki_generate_certificate](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `84.61% <66.66%> (+5.89%)` | :arrow_up: | | [target_module_vault_read](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `96.96% <100.00%> (+11.25%)` | :arrow_up: | | [target_module_vault_token_create](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `91.66% <ø> (ø)` | | | [target_module_vault_write](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `55.17% <39.39%> (-0.85%)` | :arrow_down: | | [target_modules](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `88.54% <72.13%> (-0.01%)` | :arrow_down: | | [units](https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections) | `95.53% <91.80%> (+0.38%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ansible-collections#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mathijswesterhof commented 3 months ago

@briantist I finally had some spare time to put into this again. sorry for the insane long delay. I added some base tests for _hashi_vault_common I tried to trigger code-coverage locally and it shows all tests succeed (but no new CodeCov percentages)

a few questions:

briantist commented 3 months ago

@briantist I finally had some spare time to put into this again. sorry for the insane long delay. I added some base tests for _hashi_vault_common I tried to trigger code-coverage locally and it shows all tests succeed (but no new CodeCov percentages)

  • how do I trigger the codeCov bot? (if I'm able to do that at all)

I don't think you can trigger codecov locally because you'd need a token to upload coverage reports to it, but don't worry about that. To be honest, I never run coverage locally, I make sure the tests pass locally and then let CI do it with coverage so we can get the nice reports from codecov. I realize that's not so good when it's your first contribution and you have to wait for me to approve each run, you can do a lot from a single online coverage report since you can browse it and see all the missed lines so easily.

This comment gets edited/updated each time coverage runs, or you can go to the report directly: https://app.codecov.io/gh/ansible-collections/community.hashi_vault/pull/391

Do note it takes a little time for codecov to process the uploaded reports after they are sent.

  • should i move the tests into a separate file? (I test another class, but it's in the same file so I assumed it should also go in the same test file)
  • should I write an integration test for both the _hashi_vault_plugin and _hashi_vault_module files? (I assume integration since the init is already tested now and the only addition is the try block with the exception handover)

I will have to defer these questions until I can give this a proper thorough review. I am incredibly short on time lately so I will try to get back to this as soon as I can.

mathijswesterhof commented 3 weeks ago

@briantist any updates?

briantist commented 3 weeks ago
  • should i move the tests into a separate file? (I test another class, but it's in the same file so I assumed it should also go in the same test file)

for now it's probably fine to keep them where they are, we can come back to that if it seems like they'd be better moved

  • should I write an integration test for both the _hashi_vault_plugin and _hashi_vault_module files? (I assume integration since the init is already tested now and the only addition is the try block with the exception handover)

I think they mostly get tested in integration implicitly via the integration tests for the plugins and modules that use them. The units help a lot with test cases that are hard to replicate in integration. hvac missing is one of those difficult test cases when it comes to ansible testing because ansible-test installs it for us and there isn't an easy way to tell it to do so selectively.

briantist commented 3 weeks ago

It might be good to start migrating plugins and modules to use this method. Could you do one plugin and one module and push those up so that I can review? It would be good to (roughly) have each plugin and module be in its own commit, but not strictly necessary. Thanks again for your work on this, sorry for the delays.

mathijswesterhof commented 3 weeks ago

Not sure why this one failed on the CI - LI, but main is also failing (https://github.com/ansible-collections/community.hashi_vault/actions/runs/9528624007)

Updated the KV modules and Lookups to use the HashiVaultHelper hvac entry instead of an import. (which mainly just removes the import code)

I have also removed the test that runs over the import try/except with the HAS_HVAC switch case. That test is now being done in the HashiVaultHelper test class.

I'm still doubting if it is neater to add:

    def get_hvac(self):
        return self.hvac

to the HashiVaultHelper Class and then use hvac = module.helper.get_hvac() to make it a bit more clear where HVAC is comming from. let me know if that is a good plan.

briantist commented 3 weeks ago

Not sure why this one failed on the CI - LI, but main is also failing (https://github.com/ansible-collections/community.hashi_vault/actions/runs/9528624007)

yeah the local integration tests have been failing, nothing to do with your changes, I need to get around to fixing that separately.

I'm still doubting if it is neater to add:

    def get_hvac(self):
        return self.hvac

to the HashiVaultHelper Class and then use hvac = module.helper.get_hvac() to make it a bit more clear where HVAC is comming from. let me know if that is a good plan.

I don't have a strong opinion (yet) but one way that method could be advantageous is in unit tests; it makes it easier to mock or wrap in the modules. As you said:

I have also removed the test that runs over the import try/except with the HAS_HVAC switch case. That test is now being done in the HashiVaultHelper test class.

that makes sense, but with a method we could more easily add a test to enforce that modules are actually calling the method, like:

get_hvac = mock.Mock(wraps=helper.get_hvac)
# ...
get_hvac.assert_called_once()

(something like that)

disclaimer: I haven't given it deep thought yet, feel free to play around with the different implementations

mathijswesterhof commented 3 weeks ago

I've adjusted the function a bit, since the only thing that is being called is within the exceptions subset. I personally like it better since it's more clear what it is for now. I'll also look into writing the suggested tests with the assertions probably tomorrow.