From 92d7d7caebca3f1cebabbfacb3ee90b2b874e5ab Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Thu, 27 Feb 2020 09:21:21 -0600 Subject: [PATCH] Fix W504 and remove exclusion This is a topic where there are two points of view. While neither is fundamentally better than the other in reality, what's best is to not have any arguments about it. The tox.ini comments about 503 and 504 that were in place make the argument that: - 503 is intended to be disabled and 504 enabled by default - Donald Knuth believes 504 is the right way Since Donald Knuth is smarter than all of us, align with 504, match the comments in the file and turn on enforcement to keep it that way. Change-Id: I92d4d1e82935e30ae42a0e14e641cbe36fd6e811 --- plugins/inventory/openstack.py | 6 ++-- plugins/modules/os_floating_ip.py | 10 ++++--- plugins/modules/os_ironic.py | 12 +++++--- plugins/modules/os_ironic_inspect.py | 12 +++++--- plugins/modules/os_ironic_node.py | 36 +++++++++++++++-------- plugins/modules/os_nova_host_aggregate.py | 10 ++++--- plugins/modules/os_port.py | 6 ++-- plugins/modules/os_security_group_rule.py | 33 +++++++++++++-------- plugins/modules/os_server.py | 6 ++-- plugins/modules/os_subnet.py | 7 +++-- plugins/modules/os_user.py | 6 ++-- tests/unit/mock/yaml_helper.py | 4 +-- tox.ini | 2 +- 13 files changed, 96 insertions(+), 54 deletions(-) diff --git a/plugins/inventory/openstack.py b/plugins/inventory/openstack.py index 365ae547..00a4fb58 100644 --- a/plugins/inventory/openstack.py +++ b/plugins/inventory/openstack.py @@ -184,8 +184,10 @@ class InventoryModule(BaseInventoryPlugin, Constructable, Cacheable): if not source_data: clouds_yaml_path = self._config_data.get('clouds_yaml_path') if clouds_yaml_path: - config_files = (clouds_yaml_path + - client_config.CONFIG_FILES) + config_files = ( + clouds_yaml_path + + client_config.CONFIG_FILES + ) else: config_files = None diff --git a/plugins/modules/os_floating_ip.py b/plugins/modules/os_floating_ip.py index eaa60c07..0d136c2c 100644 --- a/plugins/modules/os_floating_ip.py +++ b/plugins/modules/os_floating_ip.py @@ -185,10 +185,12 @@ def main(): network_id = None # check if we have floating ip on given nat_destination network if nat_destination: - nat_floating_addrs = [addr for addr in server.addresses.get( - cloud.get_network(nat_destination)['name'], []) - if addr['addr'] == public_ip and - addr['OS-EXT-IPS:type'] == 'floating'] + nat_floating_addrs = [ + addr for addr in server.addresses.get( + cloud.get_network(nat_destination)['name'], []) + if addr['addr'] == public_ip + and addr['OS-EXT-IPS:type'] == 'floating' + ] if len(nat_floating_addrs) == 0: module.fail_json(msg="server {server} already has a " diff --git a/plugins/modules/os_ironic.py b/plugins/modules/os_ironic.py index be94b19c..3fe55455 100644 --- a/plugins/modules/os_ironic.py +++ b/plugins/modules/os_ironic.py @@ -240,13 +240,17 @@ def main(): if not HAS_JSONPATCH: module.fail_json(msg='jsonpatch is required for this module') - if (module.params['auth_type'] in [None, 'None'] and - module.params['ironic_url'] is None): + if ( + module.params['auth_type'] in [None, 'None'] + and module.params['ironic_url'] is None + ): module.fail_json(msg="Authentication appears to be disabled, " "Please define an ironic_url parameter") - if (module.params['ironic_url'] and - module.params['auth_type'] in [None, 'None']): + if ( + module.params['ironic_url'] + and module.params['auth_type'] in [None, 'None'] + ): module.params['auth'] = dict( endpoint=module.params['ironic_url'] ) diff --git a/plugins/modules/os_ironic_inspect.py b/plugins/modules/os_ironic_inspect.py index 1cab84f1..1c1616e0 100644 --- a/plugins/modules/os_ironic_inspect.py +++ b/plugins/modules/os_ironic_inspect.py @@ -106,13 +106,17 @@ def main(): module_kwargs = openstack_module_kwargs() module = AnsibleModule(argument_spec, **module_kwargs) - if (module.params['auth_type'] in [None, 'None'] and - module.params['ironic_url'] is None): + if ( + module.params['auth_type'] in [None, 'None'] + and module.params['ironic_url'] is None + ): module.fail_json(msg="Authentication appears to be disabled, " "Please define an ironic_url parameter") - if (module.params['ironic_url'] and - module.params['auth_type'] in [None, 'None']): + if ( + module.params['ironic_url'] + and module.params['auth_type'] in [None, 'None'] + ): module.params['auth'] = dict( endpoint=module.params['ironic_url'] ) diff --git a/plugins/modules/os_ironic_node.py b/plugins/modules/os_ironic_node.py index 519bc9f7..8ea80997 100644 --- a/plugins/modules/os_ironic_node.py +++ b/plugins/modules/os_ironic_node.py @@ -194,11 +194,15 @@ def _check_set_power_state(module, cloud, node): cloud.set_machine_power_off(node['uuid']) module.exit_json(changed=True, msg="Power requested off") if 'power off' in str(node['power_state']): - if (_is_false(module.params['power']) and - _is_false(module.params['state'])): + if ( + _is_false(module.params['power']) + and _is_false(module.params['state']) + ): return False - if (_is_false(module.params['power']) and - _is_false(module.params['state'])): + if ( + _is_false(module.params['power']) + and _is_false(module.params['state']) + ): module.exit_json( changed=False, msg="Power for node is %s, node must be reactivated " @@ -207,8 +211,10 @@ def _check_set_power_state(module, cloud, node): # In the event the power has been toggled on and # deployment has been requested, we need to skip this # step. - if (_is_true(module.params['power']) and - _is_false(module.params['deploy'])): + if ( + _is_true(module.params['power']) + and _is_false(module.params['deploy']) + ): # Node is powered down when it is not awaiting to be provisioned cloud.set_machine_power_on(node['uuid']) return True @@ -234,19 +240,25 @@ def main(): module_kwargs = openstack_module_kwargs() module = AnsibleModule(argument_spec, **module_kwargs) - if (module.params['auth_type'] in [None, 'None'] and - module.params['ironic_url'] is None): + if ( + module.params['auth_type'] in [None, 'None'] + and module.params['ironic_url'] is None + ): module.fail_json(msg="Authentication appears disabled, Please " "define an ironic_url parameter") - if (module.params['ironic_url'] and - module.params['auth_type'] in [None, 'None']): + if ( + module.params['ironic_url'] + and module.params['auth_type'] in [None, 'None'] + ): module.params['auth'] = dict( endpoint=module.params['ironic_url'] ) - if (module.params['config_drive'] and - not isinstance(module.params['config_drive'], (str, dict))): + if ( + module.params['config_drive'] + and not isinstance(module.params['config_drive'], (str, dict)) + ): config_drive_type = type(module.params['config_drive']) msg = ('argument config_drive is of type %s and we expected' ' str or dict') % config_drive_type diff --git a/plugins/modules/os_nova_host_aggregate.py b/plugins/modules/os_nova_host_aggregate.py index 637161ce..dc7aac80 100644 --- a/plugins/modules/os_nova_host_aggregate.py +++ b/plugins/modules/os_nova_host_aggregate.py @@ -76,10 +76,12 @@ def _needs_update(module, aggregate): if module.params['availability_zone'] is not None: new_metadata['availability_zone'] = module.params['availability_zone'] - if ((module.params['name'] != aggregate.name) or - (module.params['hosts'] is not None and set(module.params['hosts']) != set(aggregate.hosts)) or - (module.params['availability_zone'] is not None and module.params['availability_zone'] != aggregate.availability_zone) or - (module.params['metadata'] is not None and new_metadata != aggregate.metadata)): + if ( + (module.params['name'] != aggregate.name) + or (module.params['hosts'] is not None and set(module.params['hosts']) != set(aggregate.hosts)) + or (module.params['availability_zone'] is not None and module.params['availability_zone'] != aggregate.availability_zone) + or (module.params['metadata'] is not None and new_metadata != aggregate.metadata) + ): return True return False diff --git a/plugins/modules/os_port.py b/plugins/modules/os_port.py index a7ba5908..34fad6bc 100644 --- a/plugins/modules/os_port.py +++ b/plugins/modules/os_port.py @@ -252,8 +252,10 @@ def _needs_update(module, port, cloud): if module.params[key] is not None and module.params[key] != port[key]: return True for key in compare_list: - if module.params[key] is not None and (set(module.params[key]) != - set(port[key])): + if ( + module.params[key] is not None + and set(module.params[key]) != set(port[key]) + ): return True for key in compare_list_dict: diff --git a/plugins/modules/os_security_group_rule.py b/plugins/modules/os_security_group_rule.py index 24b9a121..8f4e6156 100644 --- a/plugins/modules/os_security_group_rule.py +++ b/plugins/modules/os_security_group_rule.py @@ -223,9 +223,13 @@ def _ports_match(protocol, module_min, module_max, rule_min, rule_max): module_min = None module_max = None - if ((module_min is None and module_max is None) and - (rule_min and int(rule_min) == 1 and - rule_max and int(rule_max) == 65535)): + if ( + (module_min is None and module_max is None) + and ( + rule_min and int(rule_min) == 1 + and rule_max and int(rule_max) == 65535 + ) + ): # (None, None) == (1, 65535) return True @@ -254,16 +258,19 @@ def _find_matching_rule(module, secgroup, remotegroup): remote_group_id = remotegroup['id'] for rule in secgroup['security_group_rules']: - if (protocol == rule['protocol'] and - remote_ip_prefix == rule['remote_ip_prefix'] and - ethertype == rule['ethertype'] and - direction == rule['direction'] and - remote_group_id == rule['remote_group_id'] and - _ports_match(protocol, - module.params['port_range_min'], - module.params['port_range_max'], - rule['port_range_min'], - rule['port_range_max'])): + if ( + protocol == rule['protocol'] + and remote_ip_prefix == rule['remote_ip_prefix'] + and ethertype == rule['ethertype'] + and direction == rule['direction'] + and remote_group_id == rule['remote_group_id'] + and _ports_match( + protocol, + module.params['port_range_min'], + module.params['port_range_max'], + rule['port_range_min'], + rule['port_range_max']) + ): return rule return None diff --git a/plugins/modules/os_server.py b/plugins/modules/os_server.py index 2731179f..237ee1b2 100644 --- a/plugins/modules/os_server.py +++ b/plugins/modules/os_server.py @@ -572,8 +572,10 @@ def _check_security_groups(module, cloud, server): # server security groups were added to shade in 1.19. Until then this # module simply ignored trying to update security groups and only set them # on newly created hosts. - if not (hasattr(cloud, 'add_server_security_groups') and - hasattr(cloud, 'remove_server_security_groups')): + if not ( + hasattr(cloud, 'add_server_security_groups') + and hasattr(cloud, 'remove_server_security_groups') + ): return changed, server module_security_groups = set(module.params['security_groups']) diff --git a/plugins/modules/os_subnet.py b/plugins/modules/os_subnet.py index 24d049af..3cbc0562 100644 --- a/plugins/modules/os_subnet.py +++ b/plugins/modules/os_subnet.py @@ -272,8 +272,11 @@ def main(): if state == 'present': if not module.params['network_name']: module.fail_json(msg='network_name required with present state') - if (not module.params['cidr'] and not use_default_subnetpool and - not extra_specs.get('subnetpool_id', False)): + if ( + not module.params['cidr'] + and not use_default_subnetpool + and not extra_specs.get('subnetpool_id', False) + ): module.fail_json(msg='cidr or use_default_subnetpool or ' 'subnetpool_id required with present state') diff --git a/plugins/modules/os_user.py b/plugins/modules/os_user.py index e7763923..9b4204c8 100644 --- a/plugins/modules/os_user.py +++ b/plugins/modules/os_user.py @@ -150,8 +150,10 @@ def _needs_update(params_dict, user): # We don't get password back in the user object, so assume any supplied # password is a change. - if (params_dict['password'] is not None and - params_dict['update_password'] == 'always'): + if ( + params_dict['password'] is not None + and params_dict['update_password'] == 'always' + ): return True return False diff --git a/tests/unit/mock/yaml_helper.py b/tests/unit/mock/yaml_helper.py index cc095fea..881792db 100644 --- a/tests/unit/mock/yaml_helper.py +++ b/tests/unit/mock/yaml_helper.py @@ -106,8 +106,8 @@ class YamlTestUtils(object): assert yaml_string == yaml_string_obj_from_stream assert yaml_string == yaml_string_obj_from_stream == yaml_string_obj_from_string - assert (yaml_string == yaml_string_obj_from_stream == yaml_string_obj_from_string == yaml_string_stream_obj_from_stream == - yaml_string_stream_obj_from_string) + assert (yaml_string == yaml_string_obj_from_stream == yaml_string_obj_from_string == yaml_string_stream_obj_from_stream + == yaml_string_stream_obj_from_string) assert obj == obj_from_stream assert obj == obj_from_string assert obj == yaml_string_obj_from_stream diff --git a/tox.ini b/tox.ini index 80d60d5d..921719c1 100644 --- a/tox.ini +++ b/tox.ini @@ -51,7 +51,7 @@ commands = {posargs} # H4 are rules for docstrings. Maybe we should clean them? # E501,E402,H301,H236,F401 are ignored so we can import the existing # modules unchanged and then clean them in subsequent patches. -ignore = W503,H4,E501,E402,H301,H236,F401,W504 +ignore = W503,H4,E501,E402,H301,H236,F401 show-source = True exclude=.venv,.git,.tox,dist,doc,*lib/python*,*egg,build,ansible_collections