Add support for passing environment variables to convert2rhel during convert. #52
Conversation
roles/convert/tasks/main.yml
Outdated
| @@ -6,7 +6,7 @@ | |||
|
|
|||
| - name: Convert2RHEL convert | |||
| ansible.builtin.shell: > | |||
| export PATH={{ convert_os_path }}; | |||
| export PATH={{ convert_os_path }} {{ convert_convert2rhel_env_vars }}; | |||
There was a problem hiding this comment.
Should we move this down? I'm wondering if that could not allow for some incorrect usage, for example:
convert_convert2rhel_env_vars = "foo_bar"
...
export PATH={{ convert_os_path }} foo_bar
Thus overriding the PATH variable.
I think it should be safer if we used like:
| export PATH={{ convert_os_path }} {{ convert_convert2rhel_env_vars }}; | |
| export PATH={{ convert_os_path }}; | |
| export {{ convert_convert2rhel_env_vars }}; |
There was a problem hiding this comment.
Also, doing a quick search, it seems that ansible.builtin.shell has a environment option, maybe we could try to adapt this code to use it?
There was a problem hiding this comment.
That was my original plan, but I found the export already there and followed that lead. I'm hesitant to move that because I'm not sure I can test it, having never ran into the PATH issue myself.
@scott-vick Was there a reason you added the export like this and not using the task environment parameter? Can you share any pointers on how I could test this if I were to move it?
There was a problem hiding this comment.
I never touched that one, so not sure why it was done that way. Think you are confusing my work on infra.leapp.
There was a problem hiding this comment.
I think his question was more towards: "why are infra.leapp using export PATH=... instead of using the environment field for the shell command?"
There was a problem hiding this comment.
Hi @scott-vick,
infra.convert2rhel started out as a copy of infra.leapp as both do similar things. It was then adapted from leapp to convert2rhel. The PR I shared (redhat-cop/infra.leapp#117) is from your PR to infra.leapp which added the export to infra.leapp.
The question is the same in either case. Is there a reason that you did the export and not the environment? Seems like not, but I don't want to undo it if there's a reason for it. If there's no reason, then I'll try to move all the ENV vars to environment on the task instead of export in the command.
There was a problem hiding this comment.
I cannot remember if we tried env or not... I thought we did and it didn't work but I am not positive it would be worth testing again... but this was related to duzo issues so might be hard to test.
There was a problem hiding this comment.
Hi @scott-vick,
infra.convert2rhel started out as a copy of infra.leapp as both do similar things. It was then adapted from leapp to convert2rhel. The PR I shared (redhat-cop/infra.leapp#117) is from your PR to infra.leapp which added the export to infra.leapp.
The question is the same in either case. Is there a reason that you did the export and not the environment? Seems like not, but I don't want to undo it if there's a reason for it. If there's no reason, then I'll try to move all the ENV vars to environment on the task instead of export in the command.
When I created it, I had forgotten about the "environment" attribute being available so I just did it as an inline setting for the shell commands. If you want to change it around, I don't see any issue with it.
to convert2rhel during the convert process.
jeffmcutter
force-pushed
the
allow_env_vars_for_convert
branch
from
eb381b0
to
e6bd44a
Compare
|
I have tried moving the export $PATH to the task environment and it doesn't work with $PATH, nor does it work with not setting analysis_os_path at all and using default(omit). It does work if you explicitly set the path (analysis_os_path: /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin). That said, I don't think we want to move the export PATH. I did convert the convert_convert2rhel_env_vars to use the task environment parameter though. |
No description provided.