update logic for cloud-init and variable names for resolving ambiguity

[samvarankashyap]

$subject
#1630

@johnbieren Please check if this patch works for you.

[p3ck]

This line still looks wrong to me. Shouldn't you correct the check on run_commands and is and still the correct logic? Wouldn't it be correct if either you had users or run_commands defined?

Has this passed your unit tests for cloud-init??

Why change the variable name when it has nothing to do with the logic fix? it makes a one line patch much bigger than it needs to.

[samvarankashyap]

@p3ck
There are two ways to one can use cloud_config in linchpin
cloud_config variable that contains virt-type as one of the parameters that determine whether we use cloud-init or virt-customize. It defaults to cloud-init.
when using cloud-init does not have an option for run_commands since it is provided by virt-customize.
the above condition checks if run_commands empty or not.
Thinking of the above maybe we should check if run_commands parameter is defined or not.

I am afraid we do not have any unit tests for cloud-init but I tested it with libvirt-custom-xml target
from the libvirt workspace.

cloud_config_used variable is used to only check for the case when is cloud-init is used.
That's the reason I renamed cloud_config used to cloud_init used.

[14rcole]

The logic seems to work, but there may be a more elegant way of solving the issue. In order to determine if you run cloud-init or virt-customize, do you just check whether run_commands exists?

I do see SK's point about changing the variable name. It makes the PR longer but definitely makes the code easier to understand

[johnbieren]

"I am afraid we do not have any unit tests for cloud-init but I tested it with libvirt-custom-xml target
from the libvirt workspace." <- Is adding unit tests for it and CI testing for libvirt overall on the near term roadmap? I know of at least three teams that rely on libvirt in linchpin working and this latest release bit us

[samvarankashyap]

@johnbieren We will try to re-enable to ci for libvirt on one distro,
however, I am not sure how easy or difficult the task would be we faced multiple (networking) problems with libvirt provisioning with existing ci. Which made us disable libvirt testing.
I am currently, working on #1542 for libvirt that simulates one of the scenarios of libvirt.
provisioning which might help partially with testing.

[johnbieren]

I guess I don't follow. I have been using libvirt from linchpin for over a year now in my projects and with each linchpin release it either works exactly as I expect or doesn't work at all. There are no inconsistencies. What networking problems come up? I have not hit any networking problems ever using linchpin when I am on a working release

[p3ck]

It's no wonder I'm confused. Looking at the code there is virt_type which can be virt-customize or cloud-init, but then there is more logic in the latest version with cloud_config_used which is now called cloud_init_used.

[johnbieren]

No, this patch does not work

[johnbieren]

"msg": "AnsibleUndefinedVariable: 'cloud_config_used' is undefined"}
linchpin-workspace]# grep -r cloud_config_used ../
../lib/python3.6/site-packages/linchpin/provision/roles/libvirt/templates/libvirt_node.xml.j2:{% if virt_type == 'cloud-init' and cloud_config_used %}

[johnbieren]

ssh now works with the latest version of the patch

[14rcole]

@samvarankashyap Can you rebase this against the latest version of develop?

[samvarankashyap]

@14rcole rebased

[samvarankashyap]

[test]

[samvarankashyap]

[merge]