Add appliance/container tests for ansible-runner #23482
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1 task
Fryguy
commented
Jun 13, 2025
lib/ansible/runner.rb
Outdated
| end | ||
|
|
||
| def venv_bin_path | ||
| Dir.glob(File.join(VENV_ROOT, "bin")).first |
There was a problem hiding this comment.
This is "clever", but it's effectively a way to check the presence of a directory and return nil if not present
Fryguy
commented
Jun 13, 2025
| setup_file() { | ||
| export BATS_LIB_PATH="$HOME/.bats/libs:$BATS_LIB_PATH" | ||
|
|
||
| export PYTHON_VERSION="3.12" |
There was a problem hiding this comment.
If testing before the upgrade to python3.12, you just have to change this to 3.9. Note that aws will fail, but that's because it's actually broken right now. 🙃 This test actually found the bug.
Fryguy
force-pushed
the
add_ansible_runner_appliance_tests
branch
from
318292e to
e4288c7
Compare
Fryguy
commented
Jun 13, 2025
| return true unless requirements_file.exist? | ||
|
|
||
| require "awesome_spawn" | ||
| AwesomeSpawn.run!("ansible-galaxy", :params => ["install", {:roles_path= => roles_dir, :role_file= => requirements_file}]) | ||
| AwesomeSpawn.run!("ansible-galaxy", :env => env, :params => ["install", {:roles_path= => roles_dir, :role_file= => requirements_file}]) |
There was a problem hiding this comment.
With the new rpm changes, ansible-galaxy now lives inside the venv, so we need the venv environment in order to even run it.
Fryguy
commented
Jun 13, 2025
| @@ -221,6 +221,8 @@ def run_via_cli(hosts, credentials, env_vars, extra_vars, tags: nil, ansible_run | |||
|
|
|||
| params = runner_params(base_dir, ansible_runner_method, playbook_or_role_args, verbosity) | |||
|
|
|||
| # puts "#{env_vars_hash.map { |k, v| "#{k}=#{v}" }.join(" ")} #{AwesomeSpawn.build_command_line("ansible-runner", params)}" | |||
There was a problem hiding this comment.
This is a debug thing that I do ALL THE TIME. I'm just leaving it in the code now :)
Fryguy
commented
Jun 13, 2025
lib/ansible/runner.rb
Outdated
| end | ||
|
|
||
| def runner_env | ||
| {"PYTHONPATH" => python_path}.delete_nils | ||
| {"PYTHONPATH" => runner_python_path, "PATH" => runner_path}.compact |
There was a problem hiding this comment.
Technically, we should have been setting the path before as well, which is why this change works on the old python3.9 and the new python3.12. On either version, the virtual env activate sets both of these.
Fryguy
commented
Jun 13, 2025
| def ansible_python_version_raw | ||
| `ansible --version 2>/dev/null`.chomp | ||
| ansible = venv_bin_path ? File.join(venv_bin_path, "ansible") : "ansible" |
There was a problem hiding this comment.
Checking this allows it to run locally, on the old appliance where it doesn't live in the venv, and on the new appliance where is does live in the venv.
Fryguy
force-pushed
the
add_ansible_runner_appliance_tests
branch
from
e4288c7 to
2636165
Compare
Fryguy
commented
Jun 14, 2025
| } | ||
|
|
||
| @test "[Ansible::Runner] runs a playbook with vault encrypted variables" { | ||
| skip "requires database access" |
There was a problem hiding this comment.
So this is annoying. The way creds work through Ansible::Runner requires an Authentication record in the database, and then the interface expect the Authentication id. I don't like that and IMO, Ansible::Runner should be database agnostic. It should accept string or perhaps an object that responds_to something, but not a literal ActiveRecord id.
Because of this, on a raw container where we don't have the database, I can't run this test. The same test is above via CLI, so I'm not too worried about it.
I've already written up a change where the interface can accept a String or an object, but I want that in a separate PR.
Fryguy
force-pushed
the
add_ansible_runner_appliance_tests
branch
from
2636165 to
69fe8f3
Compare
Fryguy
changed the title
|
@bdunne This is ready to go. |
Fryguy
force-pushed
the
add_ansible_runner_appliance_tests
branch
from
69fe8f3 to
d958a8f
Compare
Fryguy
force-pushed
the
add_ansible_runner_appliance_tests
branch
from
d958a8f to
a7312a3
Compare
bdunne
reviewed
Jun 17, 2025
lib/ansible/runner.rb
Outdated
| def venv_python_path | ||
| return @venv_python_path if defined?(@venv_python_path) | ||
|
|
||
| @venv_python_path = Dir.glob(File.join(VENV_ROOT, "lib/python*/site-packages")).first |
There was a problem hiding this comment.
Did you want to change the * to 3.12 before merge?
This commit introduces Bats tests for ansible-runner to allow testing the manageiq-ansible-venv installation. This tests cover ansible-runner CLI tests as well as tests running through the Ansible::Runner Ruby class, even without database access. This commit also includes changes to Ansible::Runner itself to allow for better detection of ansible and the manageiq-ansible-venv, allowing the tests to run on the old appliances with python3.9 as well as the new appliances with python3.12.
Fryguy
force-pushed
the
add_ansible_runner_appliance_tests
branch
from
a7312a3 to
707b594
Compare
|
Checked commit Fryguy@707b594 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.62.0, and yamllint **
|
|
Some comments on commit Fryguy@707b594 lib/tasks/test_security_helper.rb
spec/lib/ansible/runner_execution_spec.bats
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This commit introduces Bats tests for ansible-runner to allow testing the manageiq-ansible-venv installation. This tests cover ansible-runner CLI tests as well as tests running through the Ansible::Runner Ruby class, even without database access.
This commit also includes changes to Ansible::Runner itself to allow for better detection of ansible and the manageiq-ansible-venv, allowing the tests to run on the old appliances with python3.9 as well as the new appliances with python3.12.
As such, this can (and should) be merged ahead of ManageIQ/manageiq-rpm_build#573.
I've executed these tests on containers and appliances before and after the RPM upgrade in ManageIQ/manageiq-rpm_build#573 both with and without sourcing the manageiq-ansible-venv.
Testing in containers you do the following:
Folow ups: