add group_by_os_family in azure dynamic inventory #40702

EtienneDeneuve · 2018-05-25T08:52:12Z

SUMMARY

Add a way to group by os family.

I need that for set correct winrm/ssh connection in group_vars

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

azure_rm.py
azure_rm.ini

ANSIBLE VERSION

ansible 2.5.3
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/neocaseadmin/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/lib/python2.7/dist-packages/ansible
  executable location = /usr/bin/ansible
  python version = 2.7.12 (default, Dec  4 2017, 14:50:18) [GCC 5.4.0 20160609]

ADDITIONAL INFORMATION

ansibot · 2018-05-25T09:03:20Z

The test ansible-test sanity --test pylint [explain] failed with 1 error:

contrib/inventory/azure_rm.py:818:0: syntax-error unindent does not match any outer indentation level (&lt;string&gt;, line 818)

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

contrib/inventory/azure_rm.py:818:35: SyntaxError: if self.group_by_os_family:

The test ansible-test sanity --test compile --python 2.7 [explain] failed with 1 error:

contrib/inventory/azure_rm.py:818:35: SyntaxError: if self.group_by_os_family:

The test ansible-test sanity --test compile --python 3.5 [explain] failed with 1 error:

contrib/inventory/azure_rm.py:818:35: SyntaxError: if self.group_by_os_family:

The test ansible-test sanity --test compile --python 3.6 [explain] failed with 1 error:

contrib/inventory/azure_rm.py:818:35: SyntaxError: if self.group_by_os_family:

The test ansible-test sanity --test compile --python 3.7 [explain] failed with 1 error:

contrib/inventory/azure_rm.py:818:35: SyntaxError: if self.group_by_os_family:

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

contrib/inventory/azure_rm.py:818:8: E901 IndentationError: unindent does not match any outer indentation level

click here for bot help

ansibot · 2018-05-25T09:03:21Z

cc @haroldwongms @nitzmahone @trstringer @xscript @yuwzho @zikalino
click here for bot help

EtienneDeneuve · 2018-05-28T06:22:09Z

Where is the doc of azure_rm inventory ? I need to update it to add what provide my few additions to initial script.

yuwzho · 2018-05-28T06:26:11Z

@EtienneDeneuve Doc is here: https://github.com/ansible/ansible/blob/devel/docs/docsite/rst/scenario_guides/guide_azure.rst

yungezz · 2018-05-28T07:11:47Z

contrib/inventory/azure_rm.py

@@ -808,10 +810,16 @@ def _add_host(self, vars):

        host_name = self._to_safe(vars['name'])
        resource_group = self._to_safe(vars['resource_group'])
+        operating_system_type = self._to_safe(vars['os_disk']['operating_system_type'])


the change looks good. pls update doc as @yuwzho mentioned.

EtienneDeneuve · 2018-05-28T09:40:19Z

@yungezz

The doc is updated.

I think there's maybe an little enhancement, for me it's better to ensure the operating_system_type is in lower case, no? I'm not a python guru but I think we can change in 2 places:

host_vars['os_disk'] = dict(
                name=machine.storage_profile.os_disk.name,
                operating_system_type=machine.storage_profile.os_disk.os_type.value
            )

by

host_vars['os_disk'] = dict(
                name=machine.storage_profile.os_disk.name,
                operating_system_type=machine.storage_profile.os_disk.os_type.value.tolower()
            )

Or in the group :

 def _add_host(self, vars):
        host_name = self._to_safe(vars['name'])
        resource_group = self._to_safe(vars['resource_group'])
        operating_system_type = self._to_safe(vars['os_disk']['operating_system_type'])
        security_group = None

by

 def _add_host(self, vars):
        host_name = self._to_safe(vars['name'])
        resource_group = self._to_safe(vars['resource_group'])
        operating_system_type = self._to_safe(vars['os_disk']['operating_system_type'].tolower())
        security_group = None

I don't know which is the best/correct way to handle that. Azure send the data with Windows or Linux, and so in group_vars, we need to create Windows.yml or Linux.yml instead of lower case.
For me, the second option is better as we don't alter the Azure output in the inventory but create the dynamic group in lower case. I wait for your advice and update the PR with the correct way.

yungezz · 2018-06-13T05:35:23Z

@EtienneDeneuve each way is ok for me. I personal prefer to lower case in _add_host.

EtienneDeneuve · 2018-06-13T07:29:53Z

I think it’s the cleaner way too. I’ll change it today Obtenez Outlook pour iOS<https://aka.ms/o0ukef>

…

________________________________ From: Yunge Zhu <notifications@github.com> Sent: Wednesday, June 13, 2018 7:36:28 AM To: ansible/ansible Cc: Etienne Deneuve; Mention Subject: Re: [ansible/ansible] add group_by_os_family in azure dynamic inventory (#40702) @EtienneDeneuve<https://github.com/EtienneDeneuve> each way is ok for me. I personal prefer to lower case in _add_host. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#40702 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AA9qkadAzLVigLcBHlLYhyFAfSuaZ8iQks5t8KTcgaJpZM4UNmdE>.

yungezz · 2018-06-19T02:19:08Z

contrib/inventory/azure_rm.py

@@ -808,10 +810,16 @@ def _add_host(self, vars):

        host_name = self._to_safe(vars['name'])
        resource_group = self._to_safe(vars['resource_group'])
+        operating_system_type = self._to_safe(vars['os_disk']['operating_system_type'].tolower())


yungezz · 2018-06-19T02:55:44Z

@jborean93 Hi Jordan, this PR adds a new option group_by_os_family to azure inventory. pls help to review when available. I've tested it locally.

yungezz · 2018-06-19T03:30:17Z

docs/docsite/rst/scenario_guides/guide_azure.rst

@@ -402,6 +404,12 @@ Here are some examples using the inventory script:
    # Execute /bin/uname on all instances in the Testing resource group
    $ ansible -i azure_rm.py Testing -m shell -a "/bin/uname -a"

+    # Execute win_ping on all Windows instances
+    $ ansible -i azure_rm.py Windows -m win_ping


update sample to use lower case os name

yungezz · 2018-07-10T09:09:00Z

@EtienneDeneuve will you update the PR per comments? I can help after you give me repo permission.

EtienneDeneuve · 2018-07-10T09:51:42Z

Oh too bad I didn’t saw your comment until yet!

I’ll fix as mentioned today!

ansibot · 2018-08-03T10:13:21Z

cc @acozine
click here for bot help

update to match lower case

EtienneDeneuve · 2018-08-03T10:52:31Z

Normally fixed the 'tolower()' by 'lower()' and change in doc

yungezz · 2018-08-09T05:27:24Z

shipit

yungezz · 2018-08-09T08:59:34Z

docs/docsite/rst/scenario_guides/guide_azure.rst

+    $ ansible -i azure_rm.py windows -m win_ping
+
+    # Execute win_ping on all Windows instances
+    $ ansible -i azure_rm.py winux -m ping


typo: linux

* devel: (30 commits) Prevent data being truncated over persistent connection socket (#43885) Fix eos_command integration test failures (#43922) Update iosxr cliconf plugin (#43837) win_domain modules: ensure Netlogon service is still running after promotion (#43703) openvswitch_db : Handle column value conversion and idempotency in no_key case (#43869) Fix typo Fix spelling of ansbile to ansible (#43898) added platform guides for NOS and VOSS (#43854) Fix download URL for yum integration test. New module for managing EMC VNX Block storage (#42945) Docker integration tests: factorize setup (#42306) VMware: datastore selection (#35812) Remove unnecessary features from cli_command (#43829) [doc] import_role: mention version from which behavior changed and fix some typos (#43843) Add source interface and use-vrf features (#43418) Fix unreferenced msg from vmware_host (#43872) set supports_generate_diff to False vyos (#43873) add group_by_os_family in azure dynamic inventory (#40702) ansible-test: Create public key creating Windows targets (#43760) azure_rm_loadbalancer_facts.py: list() takes at least 2 arguments fix (#29046) (#29050) ...

add group_by_os_family

97980f5

fix typo

29929cf

ansibot removed ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels May 25, 2018

mkrizek removed the needs_triage Needs a first human triage before being processed. label May 25, 2018

ansibleazurebot mentioned this pull request May 26, 2018

add group_by_os_family in azure dynamic inventory VSChina/ansible-issue-tracker#237

Closed

yuwzho approved these changes May 28, 2018

View reviewed changes

yungezz approved these changes May 28, 2018

View reviewed changes

yungezz reviewed May 28, 2018

View reviewed changes

update docs with new group_by_os_family

8cf9ef3

ansibot added docs This issue/PR relates to or includes documentation. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels May 28, 2018

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 5, 2018

add tolower() in add_host function to be nicer

3f11485

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 14, 2018

yungezz reviewed Jun 19, 2018

View reviewed changes

ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jun 27, 2018

Update azure_rm.py

a6d7b2b

ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Aug 3, 2018

Update guide_azure.rst

5233453

update to match lower case

yungezz reviewed Aug 9, 2018

View reviewed changes

yungezz merged commit 379fb87 into ansible:devel Aug 9, 2018

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add group_by_os_family in azure dynamic inventory #40702

add group_by_os_family in azure dynamic inventory #40702

EtienneDeneuve commented May 25, 2018

ansibot commented May 25, 2018

ansibot commented May 25, 2018

EtienneDeneuve commented May 28, 2018

yuwzho commented May 28, 2018

yungezz May 28, 2018

EtienneDeneuve commented May 28, 2018

yungezz commented Jun 13, 2018

EtienneDeneuve commented Jun 13, 2018 via email

yungezz Jun 19, 2018

yungezz commented Jun 19, 2018

yungezz Jun 19, 2018

yungezz commented Jul 10, 2018

EtienneDeneuve commented Jul 10, 2018

ansibot commented Aug 3, 2018

EtienneDeneuve commented Aug 3, 2018

yungezz commented Aug 9, 2018

yungezz Aug 9, 2018

add group_by_os_family in azure dynamic inventory #40702

add group_by_os_family in azure dynamic inventory #40702

Conversation

EtienneDeneuve commented May 25, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented May 25, 2018

ansibot commented May 25, 2018

EtienneDeneuve commented May 28, 2018

yuwzho commented May 28, 2018

yungezz May 28, 2018

Choose a reason for hiding this comment

EtienneDeneuve commented May 28, 2018

yungezz commented Jun 13, 2018

EtienneDeneuve commented Jun 13, 2018 via email

yungezz Jun 19, 2018

Choose a reason for hiding this comment

yungezz commented Jun 19, 2018

yungezz Jun 19, 2018

Choose a reason for hiding this comment

yungezz commented Jul 10, 2018

EtienneDeneuve commented Jul 10, 2018

ansibot commented Aug 3, 2018

EtienneDeneuve commented Aug 3, 2018

yungezz commented Aug 9, 2018

yungezz Aug 9, 2018

Choose a reason for hiding this comment