Modify scripts/check_galaxy to return more descriptive error message(s) #695
Conversation
…environment could be done at prepend_galaxy, just before the check_galaxy script runs.
There was a problem hiding this comment.
Thanks for contributing!
Instead of having a 2-step check here, perhaps we could leave it as a single check (running ansible-galaxy --version, like before) and instead just change the first paragraph of the error message to something more generic, such as "there was an error running the ansible-galaxy command, please use the -vvv option to examine the build output for any errors". Doesn't need to be that exact wording, but you get the idea, hopefully.
Also, yes, we should have a test for this, but your change has exposed a hole in our CI testing. The tests for these scripts live in the pulp-integration test subdirectory, but those tests are no longer run for reasons I won't go into, but I don't see any reason why those particular tests for these scripts can't be moved to our normal integration test subdirectory. I'll work on a fix for that for you so we can get this one moving along.
|
Thanks! I added some possible causes to the error message, but I'm not sure if that's necessary. |
There was a problem hiding this comment.
I like this approach.
Once PR #696 merges, and after you rebase your change to pick it up, your change will break the new test/integration/test_build.py::test_missing_ansible integration test, but that will be easily fixable. And that would be the file where you can create a new integration test for this change using the tests in that PR as a model. You'll need to include an ansible.cfg that will break ansible-galaxy in the manner you expect.
|
Test is written, sorry for the merge instead of rebase, I didn't realise GitHub would do that automatically |
There was a problem hiding this comment.
Very nice! Just a few comments inline.
Also, the example ansible.cfg contains way more info than is necessary. Typically in tests, you want the minimum amount of data that would cause the failure you are looking for to eliminate any chances of something else causing an error (either today or further in the future). Let's trim that down to just the essential to cause the failure.
|
|
||
| dependencies: | ||
| ansible_core: | ||
| package_pip: ansible-core==2.15.12 |
There was a problem hiding this comment.
I think we can just remove the version requirement here.
| ansible_core: | ||
| package_pip: ansible-core==2.15.12 | ||
| ansible_runner: | ||
| package_pip: ansible-runner==2.3.1 |
There was a problem hiding this comment.
Same here... leave off version.
| f'--container-runtime={runtime} -v3' | ||
| ) | ||
|
|
||
| assert "ERROR - 'ansible-galaxy' command not functioning as expected" in einfo.value.stdout |
There was a problem hiding this comment.
In addition to this assert, we should add another that checks for the actual error emitted from ansible-galaxy (visible in the output thanks to using -v3) to make sure it is the faulty ansible.cfg causing the error and not something else. In this case, looks like the error string we want begins with:
ERROR: Error reading config file (/etc/ansible/ansible.cfg): Source contains parsing errors:
There was a problem hiding this comment.
Thanks for getting the exact line I needed!
|
Also, regarding the merge commit, I'm not 100% certain how the GH interface is going to handle that. We'd like to keep our history clean and linear, so if you could eliminate that for me in your PR, it would be much appreciated. |
Actually, I think you can ignore this request, if you wish. The squash button should eliminate the merge commit for us. |
|
If it's fixed by the squash I don't think I'll touch it, should there be a test for a completely missing ansible-galaxy install or is the incorrect ansible.cfg test fine? |
|
Hi @brookelew Not sure if you saw, but your change is not passing our CI tests. You can see more info on the |
|
Oh no yeah I saw sorry was just a bit busy last week |
|
|
Thanks for your contribution @brookelew ! |
|
Thanks for supervising my first open source contribution @Shrews ! : D |
…s) (ansible#695) Error message from check_galaxy is updated to be more descriptive in case of ansible.cfg parsing error. (cherry picked from commit 7643d68)
Check for both existence of ansible-galaxy and verify ansible-galaxy --version works as expected in execution environment to resolve #694
Write tests?