Make inventory_hostname configurable with hostvar_expressions

[cognifloyd]

SUMMARY

Allow configuring inventory_hostname with a constructable/compose expression in hostvar_expressions.

Currently, you can add inventory_hostname to compose vars, but that is too late, as the hostname is already set. If you try, ansible with override it with a magic var to ensure that the var matches the hostname.

Resolves #104

ISSUE TYPE

• Feature Pull Request

COMPONENT NAME

azure_rm inventory plugin

ADDITIONAL INFORMATION

Example of how this could be used with tags:

hostvar_expressions:
  inventory_hostname: tags.FQDN | default(default_inventory_hostname)

[cognifloyd]

I see some basic azure_rm tests in https://github.com/ansible-collections/azure/tree/dev/tests/integration/targets/inventory_azure, but that only tests the legacy_hostnames behavior. Do you need tests for this? If so, it is not clear how to add tests or how they fit with the existing tests.

Also, is shippable even running? I don't see this PR (or anything else) running at https://app.shippable.com/github/ansible-collections/azure/dashboard

[mwinfie]

Can a maintainer review this? I think this would solve a similar issue we are having where we want to be able to pull in hosts by ip address instead of the vm name.

[AlanCoding]

I'm not saying that this is a good idea at all, but if you do this, you should at least put an item in the documentation for hostvar_expressions that explains this feature.

[AlanCoding]

I know that inventory_hostname is a magic variable in playbooks, but I would, in no way, expect that setting that hostvar would change the name of the host. That's not have I would expect magic variables will work, and I know it doesn't jive with how the inventory plugin interface is written.

Several collections have some sort of option for doing this (modifying the name as it appears to Ansible, but not the connection name).

https://github.com/ansible-collections/amazon.aws/blob/fba558015ea763e025380d54c41b3fe30e4ffb42/plugins/inventory/aws_ec2.py#L41

https://github.com/ansible-collections/google.cloud/blob/8d231272a1523f9d137ba582483cb02eb27c733e/plugins/inventory/gcp_compute.py#L45

https://github.com/ansible-collections/vmware/blob/dd8add6d842ea595f34a461e0216a01501b8f874/plugins/inventory/vmware_vm_inventory.py#L68

You'll notice that in every one of these cases, there is a top-level option for the inventory plugin called hostnames. I would strongly suggest naming it the same yourself, not because it's the best possible name, but because it's the most standard.

I would also suggest mimicking the implementation of those other collections to the point of outright copying and pasting where possible. If you want to make it "nicer", you can see my most recent PR to VMWare ansible-collections/community.vmware#253, which is for error handling, and will account for some differences with other examples.

[cognifloyd]

Thanks for the review. I'll take a look at the interface for changing hostnames in the other inventory plugins.

This was the quick and dirty implementation, now let's see if we can get a better interface to provide this feature. Without it, I'm hard-coding my ansible inventory instead of using this plugin.

[cognifloyd]

K. I copied from the vmware inventory with a few adjustments. And I rebased.

[AlanCoding]

👍 nice handling of existing behavior.

[AlanCoding]

checked out your branch and tried running, error:

name 'to_text' is not defined

applied

diff --git a/plugins/inventory/azure_rm.py b/plugins/inventory/azure_rm.py
index 899ba14..9e4050a 100644
--- a/plugins/inventory/azure_rm.py
+++ b/plugins/inventory/azure_rm.py
@@ -198,7 +198,7 @@ from ansible.module_utils.six import iteritems
 from ansible_collections.azure.azcollection.plugins.module_utils.azure_rm_common import AzureRMAuth
 from ansible.errors import AnsibleParserError, AnsibleError
 from ansible.module_utils.parsing.convert_bool import boolean
-from ansible.module_utils._text import to_native, to_bytes
+from ansible.module_utils._text import to_native, to_bytes, to_text
 from itertools import chain
 from msrest import ServiceClient, Serializer, Deserializer
 from msrestazure import AzureConfiguration

With that, I applied something similar to your example, confirmed use of custom name from tags for 1 host, rest of hosts still using default. So with that patch 👍 from me.

[cognifloyd]

Rebased on dev.

[AlanCoding]

I have any no objections, this seems like it's adopting a relatively common pattern for inventory plugins.

[cognifloyd]

@AlanCoding I think you meant "I have no objections" instead of "I have any objections". Right?

[AlanCoding]

yes, I have no objections.

[Fred-sun]

@cognifloyd Recently, I didn’t merge this PR because I was busy with other things. Can you help resolve the conflicting files? After the settlement, we will merge this PR as much as possible. Thank you!

[cognifloyd]

rebased

[laingsc]

I see this was merged into version 1.20. Is this actually using hostvar_expressions (inventory_hostname), or is it using hostnames: as a list var? Neither seems to work using latest 1.20:

---
plugin: azure.azcollection.azure_rm
include_vmss_resource_groups:
- '*'
hostvar_expressions:
  inventory_hostname: tags.Hostname | default(default_inventory_hostname)
default_host_filters: ["powerstate != \"running\""]
keyed_groups:
- prefix: tag
  key: tags

Or this:
---
plugin: azure.azcollection.azure_rm
include_vmss_resource_groups:
- '*'
hostnames:
  - tags.Hostname
default_host_filters: ["powerstate != \"running\""]
keyed_groups:
- prefix: tag
  key: tags

[AlanCoding]

Example given is

hostnames:
  - tags.vm_name
  - default  # special var that uses the default hashed name

which is similar to your 2nd attempt but not quite the same. The "tags" key should be in hostvars, print with ansible-inventory, and the sub-key needs to exist inside of that for it to work.