Python 3 compatibility error handling: use to_native(e) instead of str(e) or e.me… #26

veloutin · 2020-04-06T17:43:44Z

SUMMARY

This simply replaces the use of e.message and str(e) on exceptions to use ansible's to_native

I had to patch elasticache as it was failing for me, and patched everything while at it.

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

A bunch of different components, same fix and error handling almost everywhere.

ADDITIONAL INFORMATION

This is the original issue I had that prompted the fix.

The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_elasticache_payload_sz37cu1p/ansible_elasticache_payload.zip/ansible/modules/cloud/amazon/elasticache.py", line 209, in create
  File "/home/vince/.local/lib/python3.6/site-packages/botocore/client.py", line 316, in _api_call
    return self._make_api_call(operation_name, kwargs)
  File "/home/vince/.local/lib/python3.6/site-packages/botocore/client.py", line 626, in _make_api_call
    raise error_class(parsed_response, operation_name)
botocore.errorfactory.InvalidParameterCombinationException: An error occurred (InvalidParameterCombination) when calling the CreateCacheCluster operation: Instance type cache.r5.xlarge can only be created in a VPC.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/vince/.ansible/tmp/ansible-tmp-1585912421.616607-191146578436763/AnsiballZ_elasticache.py", line 102, in <module>
    _ansiballz_main()
  File "/home/vince/.ansible/tmp/ansible-tmp-1585912421.616607-191146578436763/AnsiballZ_elasticache.py", line 94, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File "/home/vince/.ansible/tmp/ansible-tmp-1585912421.616607-191146578436763/AnsiballZ_elasticache.py", line 40, in invoke_module
    runpy.run_module(mod_name='ansible.modules.cloud.amazon.elasticache', init_globals=None, run_name='__main__', alter_sys=True)
  File "/usr/lib/python3.6/runpy.py", line 205, in run_module
    return _run_module_code(code, init_globals, run_name, mod_spec)
  File "/usr/lib/python3.6/runpy.py", line 96, in _run_module_code
    mod_name, mod_spec, pkg_name, script_name)
  File "/usr/lib/python3.6/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/tmp/ansible_elasticache_payload_sz37cu1p/ansible_elasticache_payload.zip/ansible/modules/cloud/amazon/elasticache.py", line 540, in <module>
  File "/tmp/ansible_elasticache_payload_sz37cu1p/ansible_elasticache_payload.zip/ansible/modules/cloud/amazon/elasticache.py", line 527, in main
  File "/tmp/ansible_elasticache_payload_sz37cu1p/ansible_elasticache_payload.zip/ansible/modules/cloud/amazon/elasticache.py", line 165, in ensure_present
  File "/tmp/ansible_elasticache_payload_sz37cu1p/ansible_elasticache_payload.zip/ansible/modules/cloud/amazon/elasticache.py", line 212, in create
AttributeError: 'InvalidParameterCombinationException' object has no attribute 'message'

With this change it works.

…ssage

veloutin · 2020-04-06T17:48:55Z

As an extra note, this PR is not much more than a search and replace. I have not thoroughly tested this, and have (so far) only used the elasticache module in it. I will be gradually using the other elements as much as possible, favoring this repository over the modules shipped with ansible 2.9.5 .

tremble

Thanks for taking the time to submit this patch. I apologise for the delay in reviewing.

The vast bulk of these changes look to be handing failure modes. The long term fix is to migrate to AnsibleAWSModule and then start using fail_json_aws.

fail_json_aws(e, msg="Some description of what you were doing")

However, to avoid perfect being the enemy of good I'd suggest using it only in the module where we're already using AnsibleAWSModule (see the only module where I explicitly suggest it)

We also have a helper that lets us do "except is_boto3_error('SomeBotoError'):" rather than nested if statements. I prefer these as you're less likely to accidentally drop an Exception on the floor.

tremble · 2020-04-25T17:57:15Z

plugins/modules/ec2_vpc_endpoint.py

@@ -296,18 +297,19 @@ def create_vpc_endpoint(client, module):
            if not status_achieved:
                module.fail_json(msg='Error waiting for vpc endpoint to become available - please check the AWS console')
    except botocore.exceptions.ClientError as e:


We have a helper function (is_boto3_error_code) which I think would be better here:

from ansible_collections.amazon.aws.plugins.module_utils.aws.core import is_boto3_error_code

except is_boto3_error_code('DryRunOperation'): changed = True result = 'Would have created VPC Endpoint if not in check mode' except is_boto3_error_core('IdempotentParameterMismatch'): ... except (botocore.exceptions.ClientError, botocore.exceptions.BotoCoreError as e): module.fail_json(msg=to_native(e), exception=traceback.format_exc(),

This avoids an additional level of nesting.

tremble · 2020-04-25T18:06:45Z

plugins/modules/ecs_taskdefinition.py

@@ -277,7 +277,7 @@ def register_task(self, family, task_role_arn, execution_role_arn, network_mode,
        try:
            response = self.ecs.register_task_definition(**params)
        except botocore.exceptions.ClientError as e:
-            self.module.fail_json(msg=e.message, **camel_dict_to_snake_dict(e.response))
+            self.module.fail_json(msg=to_native(e), **camel_dict_to_snake_dict(e.response))


This module is using AnsibleAWSModule, which means we have a 'fail' method which is much easier to use and avoids the need to for the extra import.

Suggested change

self.module.fail_json(msg=to_native(e), **camel_dict_to_snake_dict(e.response))

self.module.fail_json_aws(e, msg="Failed to register task")

tremble

ansible_collections.amazon.aws.plugins.module_utils.aws.core got renamed to ansible_collections.amazon.aws.plugins.module_utils.core

Normally we'd have put a shim in place, but because we were prior to the first release of the collection only main was updated. I'm sorry about this. If you can get this fixed up I'll try following up and see if we can get this cleanup merged.

plugins/modules/ec2_vpc_endpoint.py

veloutin · 2020-08-14T14:14:49Z

@tremble I fixed it up. I grepped the rest just in case to find other refs to module_utils.aws.core.

Based on how long it took for me to respond to the feedback and requested changes, I don't think I would be in any position to complain about having to come back and fix a few imports! 😅

veloutin · 2020-08-14T18:45:53Z

I have attempted to merge, but the increasing scope of the changes is making me slightly nervous, as the scope is increasing.
I suppose it has crossed from being a simple compat fix to a broader cleanup.

A lot of the to_native calls I actually replaced by using fail_json_aws, except in a few spots where I wasn't sure (mostly different exceptions being caught). I also had to add some is_boto3_error_code.

tremble

I think using fail_json_aws definitely falls into the 'compatibility' bucket, but I can see why you're worried about scope where things have migrated over to is_boto3_error_code.

That said, I very much appreciate you being willing to follow up with this, having just mass-updated everything to AnsibleAWSModule, I would likely have been following up with some of this cleanup myself in the near future.

I pushed a missing import and a couple of comments to tell the linter to ignore warnings it was throwing, but I think we're good to merge this I just need poke the CI due to some known test flakes.

…tead of str(e) or e.me… (ansible-collections#26) * Py3 compat error handling: use to_native(e) instead of str(e) or e.message * PR comment changes, use fail_json_aws and is_boto3_error_code This commit was initially merged in https://github.com/ansible-collections/community.aws See: ansible-collections@ffe14f9

…r(e) or e.me… (ansible-collections#26) * Py3 compat error handling: use to_native(e) instead of str(e) or e.message * PR comment changes, use fail_json_aws and is_boto3_error_code

* Move module_utils out of subdir and update imports * Use relative imports * Always get dict transformations from their canonical home * Split imports by line to match tremble's other refactoring * Make import order more consistent * Temporarily restore legacy module_utils files

…r(e) or e.me… (ansible-collections#26) * Py3 compat error handling: use to_native(e) instead of str(e) or e.message * PR comment changes, use fail_json_aws and is_boto3_error_code

…r(e) or e.me… (ansible-collections#26) * Py3 compat error handling: use to_native(e) instead of str(e) or e.message * PR comment changes, use fail_json_aws and is_boto3_error_code This commit was initially merged in https://github.com/ansible-collections/community.aws See: ansible-collections@ffe14f9

Py3 compat error handling: use to_native(e) instead of str(e) or e.me…

a52d0ef

…ssage

tremble requested changes Apr 25, 2020

View reviewed changes

jillr changed the base branch from master to main July 2, 2020 19:48

Vincent Vinet added 2 commits July 28, 2020 07:59

PR comment changes, use fail_json_aws and is_boto3_error_code

e606399

Merge remote-tracking branch 'upstream/main' into python3

05eb838

tremble requested changes Jul 28, 2020

View reviewed changes

plugins/modules/ec2_vpc_endpoint.py Outdated Show resolved Hide resolved

fix moved import

f95132e

Merge remote-tracking branch 'upstream/main' into python3

594897e

tremble added 2 commits August 15, 2020 13:06

Add missing import for is_boto3_error_code

dfad544

Ignore linting warnings caused by is_boto3_error_code

aaa3326

tremble approved these changes Aug 15, 2020

View reviewed changes

tremble merged commit ffe14f9 into ansible-collections:main Aug 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python 3 compatibility error handling: use to_native(e) instead of str(e) or e.me… #26

Python 3 compatibility error handling: use to_native(e) instead of str(e) or e.me… #26

veloutin commented Apr 6, 2020

veloutin commented Apr 6, 2020

tremble left a comment •

edited

Loading

tremble Apr 25, 2020

tremble Apr 25, 2020

tremble left a comment

veloutin commented Aug 14, 2020

veloutin commented Aug 14, 2020

tremble left a comment

	self.module.fail_json(msg=to_native(e), **camel_dict_to_snake_dict(e.response))
	self.module.fail_json_aws(e, msg="Failed to register task")

Python 3 compatibility error handling: use to_native(e) instead of str(e) or e.me… #26

Python 3 compatibility error handling: use to_native(e) instead of str(e) or e.me… #26

Conversation

veloutin commented Apr 6, 2020

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

veloutin commented Apr 6, 2020

tremble left a comment • edited Loading

Choose a reason for hiding this comment

tremble Apr 25, 2020

Choose a reason for hiding this comment

tremble Apr 25, 2020

Choose a reason for hiding this comment

tremble left a comment

Choose a reason for hiding this comment

veloutin commented Aug 14, 2020

veloutin commented Aug 14, 2020

tremble left a comment

Choose a reason for hiding this comment

tremble left a comment •

edited

Loading