-
Notifications
You must be signed in to change notification settings - Fork 602
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
Make sure that ansible params check the playbook #2677
base: master
Are you sure you want to change the base?
Conversation
3ff7933
to
e0c13ad
Compare
The ansible provisioning was missed in the original "params" feature addition: A similar change to the verification would need be needed for any external scripts: i.e. to check any "path", in addition to "script" again, checking only the top path should be OK |
This PR seems to be missing a check in Lines 158 to 163 in 16fd466
|
I don't understand what this means. |
For the "script", we check the parameter - for the "path", it needs an |
The ansible provisioning supports using a separate yaml playbook, so check this file (but only the top playbook) for any parameters... The `ansible-playbook` command does not run remotely so it does not use the param.env, which means that the env is set on the command. Signed-off-by: Anders F Björklund <anders.f.bjorklund@gmail.com>
e0c13ad
to
d1d8189
Compare
I renamed the file, to look like the others. |
env := []string{} | ||
env = append(env, os.Environ()...) |
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.
Can be simplified:
env := []string{} | |
env = append(env, os.Environ()...) | |
env := os.Environ() |
The ansible provisioning supports using a separate yaml playbook,
so check this file (but only the top playbook) for any parameters...
The
ansible-playbook
command does not run remotely so it does notuse the param.env, which means that the env is set on the command.