Closed lapwingcloud closed 2 years ago
github-actions[bot]
commented
2 years ago Thank you for contribution!✨
This PR has been merged and the docs are now incorporated into main:
https://ansible-collections.github.io/community.hashi_vault/branch/main
briantist
commented
2 years ago Hi @jchenship ! Thanks for opening this MR!
Currently I'm not sure how to run unit tests locally, but I verify this on my azure vm with
Check out the Contributor guide for how to check out the collection to the proper directory structure.
From there, you can run:
ansible-test units --docker defaultIf you have trouble running the container, you can try to run the same with ansible-core>=2.11,<2.12 because the newer containers sometimes have issues running locally.
As an alternative, you can try:
ansible-test units --venv --requirementsI've kicked off a CI run, and here's the result of the unit tests: https://github.com/ansible-collections/community.hashi_vault/runs/7846893677?check_suite_focus=true
It seems at a minimum you'll need to add your requirements to tests/unit/requirements.txt so that they get installed properly when the tests are run.
Unfortunately since you're a new contributor, I have to click the button to allow CI to run for every change you push, so it will be much faster for you to get the tests running locally.
I haven't looked very deeply at the changes yet, but on the surface they look good.
In addition to unit tests, you'll also need integration tests. Since we don't have access to Azure in the CI, we can do this via MMock to mock the responses from Vault. This is how we test AWS and LDAP right now, so you can take a look at how that works.
This is described a little bit in this comment https://github.com/ansible-collections/community.hashi_vault/issues/78#issuecomment-999223445 and also in the Contributor guide.
Since you've already got a fixture in place for the units, adding the appropriate mocks in MMock should be relatively easy as well.
Minor nit: update the copyright name and date in the files you're adding.
The sanity tests in the collection have been failing in the collection recently with these errors: https://github.com/ansible-collections/community.hashi_vault/runs/7839632355?check_suite_focus=true#step:8:124
I have not had the time to investigate and fix those; they will fail for you too, you may ignore those for now, however do not ignore any other sanity test failures you see.
Sanity tests can be run locally too:
ansible-test sanity --docker defaultIt's ok if you can't get the hang of all the pieces, I can help get that stuff over the line, but it may take a little while. You've caught me at a time where I have a lot going on and will be out of town until next week, so I apologize for slow responses.
lapwingcloud
commented
2 years ago @briantist thanks a lot for the tips and feedback, I'm quite busy at work in the weekdays as well, I will continue to work on this during the weekend because
lapwingcloud
commented
2 years ago @briantist I fixed unit tests and added integration tests, they passed in CI. So requesting a review, thank you!
The only thing is there're failed sanity tests but don't seem to be related to by changes so I'm not sure what to do with it.
briantist
commented
2 years ago @jchenship thank you! This is looking great, I think nearly complete. I just landed #297 which will fix the sanity tests.
Please rebase so we can get the full CI run and see coverage.
We'll still need at least a few minor changes, but let's push up the rebase first to get the CI complete.
Small changes needed after that:
version_added: to the new optionsI would like to release the collection version 3.2.0 today or tomorrow, because it contains a bugfix that has already affected some users (#294). If we can finish this up quickly, we can include this in 3.2.0 also.
If not, I will look to release that first, and this can go into 3.3.0, which is not a problem at all, I have no issue with making a new release very quickly after the other one.
codecov[bot]
commented
2 years ago Merging #293 (7bb2964) into main (06daf8c) will increase coverage by
0.05%. The diff coverage is100.00%.
@@ Coverage Diff @@
## main #293 +/- ##
==========================================
+ Coverage 98.46% 98.52% +0.05%
==========================================
Files 71 73 +2
Lines 3464 3602 +138
Branches 301 321 +20
==========================================
+ Hits 3411 3549 +138
Misses 44 44
Partials 9 9 | Flag | Coverage Δ | |
|---|---|---|
| env_docker-default | 98.52% <100.00%> (+0.05%) |
:arrow_up: |
| integration | 80.93% <55.00%> (-0.77%) |
:arrow_down: |
| sanity | 38.77% <27.50%> (-0.34%) |
:arrow_down: |
| target_ansible-doc | 100.00% <ø> (ø) |
|
| target_auth_approle | 89.47% <ø> (ø) |
|
| target_auth_aws_iam | 50.00% <ø> (ø) |
|
| target_auth_azure | 53.84% <53.84%> (?) |
|
| target_auth_cert | 86.36% <ø> (ø) |
|
| target_auth_jwt | 91.30% <ø> (ø) |
|
| target_auth_ldap | 89.47% <ø> (ø) |
|
| target_auth_none | 100.00% <ø> (ø) |
|
| target_auth_token | 73.07% <ø> (ø) |
|
| target_auth_userpass | 85.71% <ø> (ø) |
|
| target_connection_options | 74.76% <ø> (ø) |
|
| target_controller | 82.51% <30.00%> (-1.16%) |
:arrow_down: |
| target_filter_vault_login_token | 77.77% <ø> (ø) |
|
| target_import | 38.77% <27.50%> (-0.34%) |
:arrow_down: |
| target_lookup_hashi_vault | 81.33% <ø> (ø) |
|
| target_lookup_vault_ansible_settings | 55.87% <30.00%> (-1.35%) |
:arrow_down: |
| target_lookup_vault_kv1_get | 91.30% <ø> (ø) |
|
| target_lookup_vault_kv2_get | 91.66% <ø> (ø) |
|
| target_lookup_vault_login | 88.57% <ø> (ø) |
|
| target_lookup_vault_read | 90.00% <ø> (ø) |
|
| target_lookup_vault_token_create | 78.18% <ø> (ø) |
|
| target_lookup_vault_write | 57.57% <30.00%> (-1.69%) |
:arrow_down: |
| target_module_utils | 97.02% <100.00%> (+0.30%) |
:arrow_up: |
| target_module_vault_kv1_get | 87.50% <ø> (ø) |
|
| target_module_vault_kv2_get | 87.23% <ø> (ø) |
|
| target_module_vault_login | 83.72% <ø> (ø) |
|
| target_module_vault_pki_generate_certificate | 78.72% <ø> (ø) |
|
| target_module_vault_read | 85.71% <ø> (ø) |
|
| target_module_vault_token_create | 90.00% <ø> (ø) |
|
| target_module_vault_write | 56.46% <30.00%> (-1.86%) |
:arrow_down: |
| target_modules | 77.43% <30.00%> (-1.32%) |
:arrow_down: |
| units | 95.78% <100.00%> (+0.16%) |
:arrow_up: |
Flags with carried forward coverage won't be shown. Click here to find out more.
| Impacted Files | Coverage Δ | |
|---|---|---|
| plugins/doc_fragments/auth.py | 100.00% <ø> (ø) |
|
| plugins/module_utils/_auth_method_azure.py | 100.00% <100.00%> (ø) |
|
| plugins/module_utils/_authenticator.py | 100.00% <100.00%> (ø) |
|
| ...ins/module_utils/authentication/test_auth_azure.py | 100.00% <100.00%> (ø) |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
lapwingcloud
commented
2 years ago @briantist Hey I have fixed all the things you mentioned, please have another review, and release it in 3.2.0 if it's ok, thank you! Otherwise I will continue to fix it and hopefully we can release it in 3.3.0
lapwingcloud
commented
2 years ago @briantist Yeah that looks good to me, thanks for your help to refine the PR, look forward for it to be released!
briantist
commented
2 years ago @jchenship thank you so much for this contribution! I hope you'll consider more in the future :)
This is now released as part of version 3.2.0! 🎉
SUMMARY
Fixes https://github.com/ansible-collections/community.hashi_vault/issues/78
Supports 3 types of azure auth
jwt) directlyISSUE TYPE
COMPONENT NAME
module_utils/_auth_method_azure.py
ADDITIONAL INFORMATION
Currently I'm not sure how to run unit tests locally, but I verify this on my azure vm with
jwt
service principal
managed identity
result