Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

afbjorklund
Copy link
Member

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.

@afbjorklund
Copy link
Member Author

afbjorklund commented Oct 2, 2024

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

@jandubois
Copy link
Member

This PR seems to be missing a check in

lima/hack/test-templates.sh

Lines 158 to 163 in 16fd466

INFO 'Testing that PARAM env variables are exported to all types of provisioning scripts and probes'
limactl shell "$NAME" test -e /tmp/param-boot
limactl shell "$NAME" test -e /tmp/param-dependency
limactl shell "$NAME" test -e /tmp/param-probe
limactl shell "$NAME" test -e /tmp/param-system
limactl shell "$NAME" test -e /tmp/param-user

limactl shell "$NAME" test -e /tmp/param-ansible

@jandubois
Copy link
Member

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

I don't understand what this means.

@afbjorklund
Copy link
Member Author

For the "script", we check the parameter - for the "path", it needs an os.ReadFile first

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>
@afbjorklund
Copy link
Member Author

This PR seems to be missing a check

I renamed the file, to look like the others.

@AkihiroSuda AkihiroSuda requested review from jandubois and a team October 11, 2024 06:41
Comment on lines +67 to +68
env := []string{}
env = append(env, os.Environ()...)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplified:

Suggested change
env := []string{}
env = append(env, os.Environ()...)
env := os.Environ()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants