ansible_extra_vars & ansible_common_vars usages should be flipped?

[kkeshavamurthy]

What steps did you take and what happened:
[A clear and concise description on how to REPRODUCE the bug.]

https://github.com/kubernetes-sigs/image-builder/blob/master/images/capi/packer/ova/packer-node.json#L415-L416
Here I see ansible_extra_vars populated with some vars from packer-common.json and ansible_common_vars set to empty.

Shouldn't it be the otherway? That way users can pass extra ansible vars using ansible_extra_vars

What did you expect to happen:

ansible_common_vars --> guestinfo_datasource_* vars
ansible_extra_vars --> user configurable

Anything else you would like to add:
[Miscellaneous information that will assist in solving the issue.]

Environment:

Project (Image Builder for Cluster API, kube-deploy/imagebuilder, konfigadm):

Additional info for Image Builder for Cluster API related issues:

• OS (e.g. from /etc/os-release, or cmd /c ver):
• Packer Version:
• Packer Provider:
• Ansible Version:
• Cluster-api version (if using):
• Kubernetes version: (use kubectl version):

/kind bug
[One or more /area label. See https://github.com/kubernetes-sigs/cluster-api/labels?q=area for the list of labels]

@codenrhoden

[codenrhoden]

I do believe this is structured as intended, but there is room for improvement.

ansible_common_args is meant to be the complete set of all Ansible args that all builders are required to pass to the Ansible roles -- thus they are "common" to all builders. That's why they are defined in one single place, at images/capi/packer/config/ansible-args.json. Builders should not be overriding this -- so a potential improvement that could be made (if it works) is to just delete ansible_common_args from every packer.json file. It serves no purpose, and makes it seem like you can set it (you shouldn't). A more prescriptive version of this would be to set it to null instead of "" -- which would require that ansible-args.json is passed as a Packer var file -- which is exactly what we want. I think there were historical reasons why this wasn't setup that way to begin with (like not every builder using the Makefile) but we could do it now.

ansible_extra_vars are meant to be builder specific. OVA builder passes some stuff that is only used by OVA/vSphere Ansible roles (like the guestinfo datasource info), the flatcar builds pass stuff about Python interpreter locations, etc. So these are the "extra" configs that builders can have. I think we want to keep that convention.

But we could likely eliminate some confusion around the "common" vars. Hope what I explained makes sense.

[kkeshavamurthy]

I understand.
If I want to pass additional vars to ansible, then I need to append to ansible_extra_vars instead of just defining ansible_extra_vars: foo=bar which seems a bit hacky.

What do you think of defining ansible_user_vars and pass it like this:
"extra_arguments": [
"--extra-vars",
"{{user ansible_common_vars}}",
"--extra-vars",
"{{user ansible_extra_vars}}",
"--extra-vars",
"{{user ansible_user_vars}}"
],

[codenrhoden]

@kkeshavamurthy yeah, I think that would make sense. What is there right now doesn't give users a way to pass custom variables to Ansible at all. The only way to do it would be to do append to ansible_extra_vars, which is in direct opposition to the "don't edit the JSON files" goal.