community.hashi_vault

community.hashi_vault copied to clipboard

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

• It is not perse a bugfix but more of an open todo still in the code.
• fixing this todo opens up a lot of cleaning in the separate modules where this is being double checked.

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

• Bugfix Pull Request

COMPONENT NAME

• module_utils/_hashi_vault_common.py
• module_utils/_hashi_vault_module.py
• plugin_utils/_hashi_vault_plugin.py

ADDITIONAL INFORMATION

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

vault_list

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"}

Jul 22 '23 22:07 mathijswesterhof

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

Jul 22 '23 23:07 mathijswesterhof

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!

Jul 23 '23 10:07 briantist

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.21%. Comparing base (a5c15bf) to head (38e18f0). Report is 5 commits behind head on main.

@@           Coverage Diff            @@
##             main     #391    +/-   ##
========================================
  Coverage   99.20%   99.21%            
========================================
  Files         109      110     +1     
  Lines        6201     5767   -434     
  Branches     1184     1088    -96     
========================================
- Hits         6152     5722   -430     
+ Misses         39       36     -3     
+ Partials       10        9     -1     

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

Dec 26 '23 05:12 codecov[bot]

@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:

• 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)
• how do I trigger the codeCov bot? (if I'm able to do that at all)
• 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)

Mar 17 '24 22:03 mathijswesterhof

@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.

Mar 18 '24 23:03 briantist

@briantist any updates?

Jun 12 '24 22:06 mathijswesterhof

• 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.

Jun 13 '24 16:06 briantist

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.

Jun 13 '24 16:06 briantist

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.

Jun 15 '24 15:06 mathijswesterhof

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

Jun 15 '24 16:06 briantist

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.

Jun 15 '24 17:06 mathijswesterhof

@briantist I've reverted the exceptions function and implemented the get_hvac().exceptions like you suggested. I've also did this for all implementations i could find. Currently the pipeline is failing on Devel and after some digging it looks like it failed in a function inside the Ansible base code So I'm not sure if it will get fixed after a few commits or if that is something we should work around. I'll keep tabs on that this week. I want to get started with the new ansible functions next week so if I can get a review before then that would be amazing. otherwise it'll have to wait untill mid september

Jul 28 '24 20:07 mathijswesterhof

@mathijswesterhof the devel failures are not related to this, that needs to be fixed separately for the repo.

I want to get started with the new ansible functions next week so if I can get a review before then that would be amazing. otherwise it'll have to wait untill mid september

Unfortunately I don't think it'll be possible. It's an incredibly busy time for me and I have far less time for this than I have in the past, hoping it eases up but looking at my week ahead it feels unlikely I'll be able to take a look before then, we'll see though.

Jul 29 '24 20:07 briantist

@briantist any update?

Sep 30 '24 11:09 mathijswesterhof

The CI for the repo has been fixed, so let's get a new CI run after you update your branch

Sep 30 '24 17:09 briantist

@briantist it seems like all checks are successful (apart from the codecov upload timeout) and there are no instances of the import anymore.

Oct 01 '24 16:10 mathijswesterhof