-
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
tests - massive test refactor #106
tests - massive test refactor #106
Conversation
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, thanks for creating this documentation @briantist!
I made a few suggestions for making it even better. Incorporate the changes you like and ignore the ones you don't like.
@acozine thank you so much for the review! Your comments sparked a whole set of updates to the docs, in both formatting and content. If you have some time I'd really appreciate another look. I'm also attaching rendered HTML versions of each of the pages, though the |
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.
Overall looks great. I let my inner copyeditor loose on the text. As always, keep the changes you like and ignore the ones you don't.
Oh, one warning - if you accept any changes to headings. you'll need to extend the underlining to match, since I didn't make suggestions on those lines.
Thank you! Co-authored-by: Alicia Cozine <[email protected]>
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.
Docs look great, thanks @briantist. I found a few last nits.
.. _ansible_collections.community.hashi_vault.docsite.contributor_guide: | ||
|
||
***************** | ||
Contributor guide |
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.
Hm, the GitHub standard is to add a contributing.md
file, right? Maybe add one and point to this file or to the built page?
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.
I will leave contributing.md
for a later time, but I have updated the (previously blank) section in the README.
Co-authored-by: Alicia Cozine <[email protected]>
SUMMARY
This is a big refactor of the integration tests that I've been trying to do in smaller bits and pieces for many months. Some of the more difficult and entangled aspects were not possible or practical to do in smaller increments, and I failed several times trying in previous PRs. So as much as I prefer avoid such giant changes, this was an area I found difficult to make meaningful progress otherwise.
Changes
runme.sh
tests, and now use the "standard" role-based mechanism.setup_
roles.integration_config.yml
is now required.integration_config.yml.sample
file is included. It can be copied directly tointegration_config.yml
to go right back to the old style all-in-one setup. This will prove to be less and less viable for running all tests as more targets get added, because that process will be repeated for every target, so it's best for one-time/infrequent contributors who will run only a single target locally.community.docker
tasks (using the already includeddocker-compose
CLI), in order to remove a dependency oncommunity.crypto
andcommunity.docker
in the "main" CI test runs, and avoid the time taken to run those tasks.ansible-test
with--docker
and with--venv
. These tests run onubuntu
andmacos
, to try to ensure that we're providing a good experience for contributors.community.crypto
,community.docker
, andcommunity.general
(macos only). Since installing from Galaxy can still be error prone, I've chosen to continue installing directly from GitHub, but I've wrapped this up into a neat GitHub Action as well, so it's quite clean in the workflow.What's missing?
There's a few things that are NOT done yet in this PR
Fixes #19 (that issue is barely relevant anymore, but the tests that now run on MacOS did surface a few issues that are now fixed)
ISSUE TYPE
COMPONENT NAME
ADDITIONAL INFORMATION