Fixes #35456 - Source environment before starting ansible-runner #66
Conversation
| @@ -186,10 +189,12 @@ def write_ssh_key | |||
| def start_ansible_runner | |||
| env = {} | |||
There was a problem hiding this comment.
As mentioned in theforeman/puppet-foreman_proxy#777 (comment) I wonder if we should present everything as environment variables by default. In that case I'd probably extract the env var construction to a separate method.
| @@ -10,6 +10,9 @@ class AnsibleRunner < ::Proxy::Dynflow::Runner::Parent | |||
| include ::Proxy::Dynflow::Runner::ProcessManagerCommand | |||
| attr_reader :execution_timeout_interval | |||
|
|
|||
| # To make this overridable in development | |||
| ENVIRONMENT_WRAPPER = ENV['SMART_PROXY_ANSIBLE_ENVIRONMENT_WRAPPER'] || '/usr/libexec/foreman-proxy/ansible-runner-environment' | |||
There was a problem hiding this comment.
This is missing the .sh file extension
There was a problem hiding this comment.
It really shouldn't ;)
#66 (comment)
| @@ -186,10 +189,12 @@ def write_ssh_key | |||
| def start_ansible_runner | |||
| env = {} | |||
| env['FOREMAN_CALLBACK_DISABLE'] = '1' if @rex_command | |||
| command = [env, 'ansible-runner', 'run', @root, '-p', 'playbook.yml'] | |||
| env['ANSIBLE_ENVIRONMENT_FILE'] = Proxy::Ansible::Plugin.settings[:ansible_environment_file] | |||
There was a problem hiding this comment.
Is ANSIBLE_ENVIRONMENT_FILE an Ansible configuration value or something we are making up? I ask because if this is something we are making up we should probably avoid using the ANSIBLE_ prefix as that is a "known" pattern amongst Ansible configuration values when provided via environment variable.
There was a problem hiding this comment.
It is something I made up
There was a problem hiding this comment.
A thought to avoid potential issues there would be to use your prefix from above SMART_PROXY_ANSIBLE
There was a problem hiding this comment.
To me SMART_PROXY_ANSIBLE_ENVIRONMENT_FILE reads like it is a file that should contain environment for smart-proxy-ansible, not ansible spawned by smart-proxy-ansible, but sure, why not
There was a problem hiding this comment.
To me
SMART_PROXY_ANSIBLE_ENVIRONMENT_FILEreads like it is a file that should contain environment for smart-proxy-ansible, not ansible spawned by smart-proxy-ansible, but sure, why not
Fair. Naming is hard. Maybe @evgeni has an opinion as our resident expert on all things "fun" that can get us in trouble with Ansible.
There was a problem hiding this comment.
I think SMART_PROXY_ANSIBLE_ is a good prefix for config options for the smart_proxy_ansible plugin. In a container you'd probably do the same thing for all options.
There was a problem hiding this comment.
Done
Did you mean to push up new changes?
There was a problem hiding this comment.
Erm, yes, sorry. It is out now
Co-authored-by: Ewoud Kohl van Wijngaarden <ewoud@kohlvanwijngaarden.nl>
|
Merging, thanks @adamruzicka @ekohl @ehelms ! |
No description provided.