-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
#998 Execute task-template within python-virtual environment and ensu… #1001
base: develop
Are you sure you want to change the base?
Conversation
…nt and ensure sensitive environment variable are not passed to ansible-playbook command
Based on idea #998 this implementation allow to run the semaphore ansible task within a prepared python virtual environment '.venv' (default naming per convention for python venv) Note: In order to prepare the venv, one still has to use git post-checkout and post-merge hooks #998 (comment). This is not covered by any pull-request as it works as is from semaphore git checkout |
Hi @ccuz, sorry for delay. I reviewing your PR, thank you! |
It looks very interesting. |
Is there planned support for multiple venvs? It would be a great feature to add |
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 want to agree with the notion of cleaning up/removing ENV vars that could leak information, but I'd prefer it if that concern got separated from the virtualenv work.
@@ -16,17 +16,36 @@ type AnsiblePlaybook struct { | |||
} | |||
|
|||
func (p AnsiblePlaybook) makeCmd(command string, args []string, environmentVars *[]string) *exec.Cmd { | |||
cmd := exec.Command(command, args...) //nolint: gas | |||
commandToExec := command | |||
cmdInPythonDefaultVenv := fmt.Sprintf("%s/.venv/bin/%s", p.GetFullPath(), command) |
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 is unlikely to work nicely with every Ansible setup since playbooks could be nested into subdirectories of the ansible project.
Consider the following project setup:
. # Project Root
roles/
roles/roleA
roles/roleB
roles/roleC
playbooks/
playbooks/projectA
playbooks/projectA/playbookA.yml
playbooks/projectB/playbookB.yml
If your task template would reference playbooks/projectA/playbookA.yml
then p.GetFullPath()
may now resolve to (assuming other things being default) /tmp/semaphore/project_1/playbooks/projectA/
, which will not be where your .venv would be.
Instead I'd recommend having the local venv dependencies deployed into the $PATH
through the either activating it prior to execution or into the env that semaphore is ultimately running in.
My current installation(s) use the core semaphore container but install addtl. dependencies into the container.
Yes, this requires to update the container if you have a new dependency that needs to be installed but it does get around these presumptive and rather brittle constructs.
…re sensitive environment variable are not passed to ansible-playbook command