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

Update /plugins/inventory/__init__.py - Removed redundant "_" separator when prefix is an empty string #57164

Open
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
7 participants
@amirwollman
Copy link

commented May 30, 2019

SUMMARY

GCP dynamic inventory - removed separator in case prefix is an empty string.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

inventory plugin

ADDITIONAL INFORMATION

Added conditional before setting the separator (default '_') which sets it to '' in case prefix is '' as well.

Before change

 "all": {
     "children": [
         "__built_in_method_items_of_dict_object_at_0x2bc4db0_",
         "_analytics",
         "_consul_server",
         "_dashboardapi",
         "_elasticsearch",
         "_http_server",
         "_https_server",
         "_inventory",
         "_k2bq",
         "_kafka",
         "_kibana",
         "_logstash_k2bq",
         "_logstash_k2es",
         "_mysql",
         "_neo4j",
         "_ssl_offload",
         "_zookeeper",
         "ungrouped"
     ]

After change

 "all": {
     "children": [
         "_built_in_method_items_of_dict_object_at_0x2bc4db0_",
         "analytics",
         "consul_server",
         "dashboardapi",
         "elasticsearch",
         "http_server",
         "https_server",
         "inventory",
         "k2bq",
         "kafka",
         "kibana",
         "logstash_k2bq",
         "logstash_k2es",
         "mysql",
         "neo4j",
         "ssl_offload",
         "zookeeper",
         "ungrouped"
     ]
Update __init__.py
Added conditional to remove separator (set it to '' instead of default '_') in case prefix is '' as well. This is to avoid inventory names from adding _ before each group name.

Before changes:
"all": {
     "children": [
         "__built_in_method_items_of_dict_object_at_0x2bc4db0_",
         "_analytics",
         "_consul_server",
         "_dashboardapi",
         "_elasticsearch",
         "_http_server",
         "_https_server",
         "_inventory",
         "_k2bq",
         "_kafka",
         "_kibana",
         "_logstash_k2bq",
         "_logstash_k2es",
         "_mysql",
         "_neo4j",
         "_ssl_offload",
         "_zookeeper",
         "ungrouped"
     ]

After changes:
"all": {
     "children": [
         "_built_in_method_items_of_dict_object_at_0x2bc4db0_",
         "analytics",
         "consul_server",
         "dashboardapi",
         "elasticsearch",
         "http_server",
         "https_server",
         "inventory",
         "k2bq",
         "kafka",
         "kibana",
         "logstash_k2bq",
         "logstash_k2es",
         "mysql",
         "neo4j",
         "ssl_offload",
         "zookeeper",
         "ungrouped"
     ]

@amirwollman amirwollman changed the title Update __init__.py Update /plugins/inventory/__init__.py - Removed redundant "_" separator when prefix is an empty string May 30, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

The test ansible-test sanity --test pylint [explain] failed with 34 errors:

lib/ansible/plugins/inventory/__init__.py:405:0: syntax-error invalid syntax (<unknown>, line 405)
lib/ansible/plugins/inventory/advanced_host_list.py:28:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/auto.py:26:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/aws_ec2.py:148:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/aws_rds.py:68:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/azure_rm.py:179:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/cloudscale.py:78:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/constructed.py:76:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/docker_swarm.py:146:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/foreman.py:77:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/gcp_compute.py:126:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/generator.py:80:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/gitlab_runners.py:77:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/hcloud.py:91:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/host_list.py:32:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/ini.py:81:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/k8s.py:119:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/linode.py:61:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/netbox.py:121:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/nmap.py:61:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/online.py:66:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/openstack.py:116:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/scaleway.py:88:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/script.py:49:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/toml.py:102:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/tower.py:104:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/virtualbox.py:61:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/vmware_vm_inventory.py:102:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/vultr.py:87:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
lib/ansible/plugins/inventory/yaml.py:74:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
test/integration/targets/collections/collections/ansible_collections/testns/content_adj/plugins/inventory/statichost.py:20:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
test/integration/targets/inventory_plugin_config/test_inventory.py:31:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
test/integration/targets/old_style_cache_plugins/plugins/inventory/test.py:15:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'
test/integration/targets/rel_plugin_loading/subdir/inventory_plugins/notyaml.py:67:0: syntax-error Cannot import 'ansible.plugins.inventory' due to syntax error 'invalid syntax (<unknown>, line 405)'

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with the error:

Command "ansible-doc -t inventory advanced_host_list auto aws_ec2 aws_rds azure_rm cloudscale constructed docker_swarm foreman gcp_compute generator gitlab_runners hcloud host_list ini k8s kubevirt linode netbox nmap online openshift openstack scaleway script toml tower virtualbox vmware_vm_inventory vultr yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (__init__.py, line 405)

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with the error:

Command "ansible-doc -t inventory advanced_host_list auto aws_ec2 aws_rds azure_rm cloudscale constructed docker_swarm foreman gcp_compute generator gitlab_runners hcloud host_list ini k8s kubevirt linode netbox nmap online openshift openstack scaleway script toml tower virtualbox vmware_vm_inventory vultr yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (__init__.py, line 405)

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with the error:

Command "ansible-doc -t inventory advanced_host_list auto aws_ec2 aws_rds azure_rm cloudscale constructed docker_swarm foreman gcp_compute generator gitlab_runners hcloud host_list ini k8s kubevirt linode netbox nmap online openshift openstack scaleway script toml tower virtualbox vmware_vm_inventory vultr yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (__init__.py, line 405)

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with the error:

Command "ansible-doc -t inventory advanced_host_list auto aws_ec2 aws_rds azure_rm cloudscale constructed docker_swarm foreman gcp_compute generator gitlab_runners hcloud host_list ini k8s kubevirt linode netbox nmap online openshift openstack scaleway script toml tower virtualbox vmware_vm_inventory vultr yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (__init__.py, line 405)

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with the error:

Command "ansible-doc -t inventory advanced_host_list auto aws_ec2 aws_rds azure_rm cloudscale constructed docker_swarm foreman gcp_compute generator gitlab_runners hcloud host_list ini k8s kubevirt linode netbox nmap online openshift openstack scaleway script toml tower virtualbox vmware_vm_inventory vultr yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (__init__.py, line 405)

The test ansible-test sanity --test ansible-doc --python 3.8 [explain] failed with the error:

Command "ansible-doc -t inventory advanced_host_list auto aws_ec2 aws_rds azure_rm cloudscale constructed docker_swarm foreman gcp_compute generator gitlab_runners hcloud host_list ini k8s kubevirt linode netbox nmap online openshift openstack scaleway script toml tower virtualbox vmware_vm_inventory vultr yaml" returned exit status 250.
>>> Standard Error
ERROR! Unexpected Exception, this is probably a bug: invalid syntax (__init__.py, line 405)

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

lib/ansible/plugins/inventory/__init__.py:405:35: SyntaxError: if prefix = '':

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

lib/ansible/plugins/inventory/__init__.py:405:35: SyntaxError: if prefix = '':

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

lib/ansible/plugins/inventory/__init__.py:405:35: SyntaxError: if prefix = '':

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

lib/ansible/plugins/inventory/__init__.py:405:35: SyntaxError: if prefix = '':

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

lib/ansible/plugins/inventory/__init__.py:405:35: SyntaxError: if prefix = '':

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

lib/ansible/plugins/inventory/__init__.py:405:35: SyntaxError: if prefix = '':

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels May 30, 2019

@goneri goneri removed the needs_triage label May 30, 2019

@goneri goneri requested a review from bcoca May 30, 2019

@bcoca
Copy link
Member

left a comment

I do think the new behaviour is 'nicer' but this would require a configuration toggle, that defaults to current behaviour, any change on the default would have to go through a deprecation period

@amirwollman

This comment has been minimized.

Copy link
Author

commented Jun 1, 2019

Just noticed now my commit is missing an equal (=) sign. But by your comment @bcoca I'm assuming there is more to be done here.
I'm a very new contributor and am not entirely familiar with the best practices applied here, so if there's a manual or guidelines document you can refer me to, I'll do my very best to follow these.

P.S I'll go ahead and patch my last commit just so it will at least pass the compilation tests.

Update __init__.py
Added missing "=" in line 405
@s-hertel

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@amirwollman This is a nice improvement. :-)

I don't know if there's documentation on keeping backwards compatibility. Since the separator is used currently by Ansible regardless of whether or not a prefix is specified and only omitted with separator: "", people using this behavior will have their playbooks potentially broken by this PR. To take away behavior a deprecation period needs to be used so people are warned and have time to update their setup without things breaking. You should add a toggle to enable this behavior, but keep the current behavior as the default (optionally adding a deprecation period to change the default). You can add a toggle here: https://github.com/ansible/ansible/blob/devel/lib/ansible/config/base.yml (check out the other INVENTORY related toggles for example), and then you can access that toggle by importing ansible.constants as C and using C.TOGGLE_NAME.

@kustodian

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2019

Maybe we could introduce a new parameter which would toggle this. That way we wouldn't have to worry about depreciation or backward compatibility. Not sure about the name, maybe "trailing_separator"?

@ansibot ansibot added the stale_ci label Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.