-
Notifications
You must be signed in to change notification settings - Fork 59
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
Add cert auth method #159
Add cert auth method #159
Conversation
Hi @devon-mar ! Thank you and welcome! I want to first say say how much I appreciate the thoroughness of this contribution and working from the existing auth methods. Although it's on my list, there currently isn't good contributor documentation for this yet, so I'm very interested in hearing your thoughts and feedback on that. For the addition itself: I've taken a cursory look and it mostly seems great. There are a couple of changes I will recommend when I have a block of time to look more thoroughly. Besides that, you'll need a changelog fragment (this can be attributed to So once that lands, I will ask you to rebase this and modify the integration tests accordingly. I can also help with any of those additional changes needed, so it's not all on you. Thank you again! |
Codecov Report
@@ Coverage Diff @@
## main #159 +/- ##
==========================================
+ Coverage 89.56% 89.99% +0.42%
==========================================
Files 35 37 +2
Lines 1399 1469 +70
Branches 105 113 +8
==========================================
+ Hits 1253 1322 +69
- Misses 135 136 +1
Partials 11 11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
@devon-mar #155 has been merged, if you wouldn't mind, please rebase from You may also want to have a look at #156 to see what's coming up, but that's no worry. If that merges before this I can help fix up whatever small changes are needed. |
@briantist I think it would also be useful to have some documentation on how to setup a local development environment (running tests, etc). IMO community.zabbix has, a pretty good Also, adding some linting would be useful to keep future PRs consistent. I could take a shot at it in a different PR if you want. |
I'll try to take a look at it later this week. |
Thank you, this is great feedback! For running the integration tests locally, I have published this page on the docsite: https://docs.ansible.com/ansible/latest/collections/community/hashi_vault/docsite/contributor_guide.html#running-tests-locally There are two primary methods of getting the local Vault server setup:
Both methods take care of certificates and starting a Vault server with TLS, but I highly recommend trying the docker method locally. If you run into issues setting that up I'd like to hear about it so improvements can be made (and PRs to both that setup and the docs are welcome!). I do apologize about the difficulty of finding the HTTPS vars and such. I plan on writing guides specifically on how to contribute new auth methods, new plugins and modules, new connection options, etc. You happened to pick quite an interesting auth method! For the moment, only the connection options target needed Vault to run with TLS, but even with the difficulty I'm glad the existing vars worked out. There is a Contributing section in the README, but I'll look at adding a
What kind of linting are you thinking about? Something beyond the Ansible sanity tests? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some changes needed and other suggestions, but overall looks very good, thank you!
The target integration tests need to be updated to use the module rather than the lookup, and I can help with that if needed.
In addition, you'll need to update the argspec in the authenticator class for this to work with modules (I can't add that inline with GitHub's review options).
tests/unit/plugins/module_utils/authentication/test_auth_cert.py
Outdated
Show resolved
Hide resolved
tests/unit/plugins/module_utils/authentication/test_auth_cert.py
Outdated
Show resolved
Hide resolved
tests/unit/plugins/module_utils/authentication/test_auth_cert.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Perfect! Ok, yeah I have that issue open in order to explore that, as time permits. If you have further thoughts or specifics on that your comments in the issue would be appreciated!
Oh excellent! So glad to hear that!
|
tests/unit/plugins/module_utils/authentication/test_auth_cert.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Co-authored-by: Brian Scholer <1260690+briantist@users.noreply.github.com>
Docker. I think most of the other collections and Ansible only support Docker these days so it makes sense to remove the legacy method.
Yeah, they all worked fine. Maybe you could add the commands for units and sanity to the docs as well (these are what I'm using):
I think this PR should be good to go. I ran sanity, unit, and integration tests locally and they all pass. Let me know if there is anything else that need changing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few other small nits (I will apply the suggestions)
There are a few unresolved comments/requested changes still: |
##Docs Build 📝 Thank you for contribution!✨ This PR has been merged and the docs are now incorporated into |
Sorry about that, somehow missed them. Tests look all good on my side. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! Thank you for taking the time to contribute and for your valuable feedback!
@devon-mar if you have the time and would like to help me get No obligation of course 😅 |
SUMMARY
This PR adds the certificate auth method to the
hashi_vault
lookup plugin.ISSUE TYPE
COMPONENT NAME
hashi_vault
lookupADDITIONAL INFORMATION