ec2_vpc_igw: migrate to boto3 #45346

zeenlym · 2018-09-07T12:04:34Z

SUMMARY

AWS boto only modules present issues with corporate internet proxies. Migrate to boto3 solves problems. Code is simplier and more ansible aws helpers are used.

Fixes #44889

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

ANSIBLE VERSION

2.5 and 2.6

ADDITIONAL INFORMATION

ansibot · 2018-09-07T12:43:44Z

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

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:125:62: undefined-variable Undefined variable 'to_native'
lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:125:86: undefined-variable Undefined variable 'traceback'

The test ansible-test sanity --test pep8 [explain] failed with 6 errors:

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:88:9: E261 at least two spaces before inline comment
lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:106:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:122:37: E128 continuation line under-indented for visual indent
lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:158:9: E303 too many blank lines (3)
lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:230:5: E303 too many blank lines (2)
lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:286:1: E302 expected 2 blank lines, found 1

click here for bot help

ansibot · 2018-09-07T12:43:45Z

cc @willthames
click here for bot help

ansibot · 2018-09-07T12:43:45Z

Hi @zeenlym,

Thank you for the pullrequest, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

s-hertel

I'd be in favor of not having a class in the module just to simplify things a little but LGTM even with it. There should be integration tests added to help ensure backward compatibility.

s-hertel · 2018-09-07T14:19:34Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

-            'tags': igw.tags,
-            'vpc_id': igw.vpc_id
-            }
+class AnsibleEc2Igw(object):


Most AWS modules don't implement classes as using a few functions is often more readable.

I find it easier to read within a class, and using attributes prevent from passing lots of parameters to functions.
I think if all AWS modules uses classes even if module complexity is very high or very low readability will be the same.

s-hertel · 2018-09-07T14:21:43Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

-def get_resource_tags(vpc_conn, resource_id):
-    return dict((t.name, t.value) for t in
-                vpc_conn.get_all_tags(filters={'resource-id': resource_id}))
+    def connect(self):


You don't need to access get_aws_connection_info and boto3_conn directly and any errors with the connection should be handled by boto3_conn. After creating the module by module = AnsibleAWSModule(... the connection can be created with module.client('ec2').

s-hertel · 2018-09-07T14:26:22Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

+            igws = response['InternetGateways']
+        except botocore.exceptions.ClientError as e:
+            self.fail(msg=str(e))
+


Remove some extra whitespace

s-hertel · 2018-09-07T14:34:00Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

-    if igw is None:
-        if check_mode:
-            return {'changed': True, 'gateway_id': None}
+        self.start_timer()


This doesn't appear to do anything for the module.

That's rigth, sorry for this

s-hertel · 2018-09-07T14:45:48Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py


-    igw.vpc_id = vpc_id
+        if to_delete and not add_only:


I think rather than checking for to_delete and add_only here (and having two hardcoded values for add_only and the purge_tags parameter for compare_aws_tags), I'd consolidate the options since they do the same thing and only have one hardcoded value:

purge_tags = bool(not add_only) to_update, to_delete = compare_aws_tags(boto3_tag_list_to_ansible_dict(cur_tags.get('Tags')), tags, purge_tags)

And then to_delete will only be populated if tags are being removed and you don't have to reference add_only in other places.

s-hertel · 2018-09-07T14:46:42Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

+
+from ansible.module_utils.aws.core import AnsibleAWSModule
+from ansible.module_utils.ec2 import (
+    AnsibleAWSError,


You can remove this line.

willthames · 2018-09-10T09:37:54Z

@s-hertel has said what I would say in regard to integration tests.

willthames

A few minor issues but nothing that would stop this module working

willthames · 2018-09-10T09:41:54Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

+                        tags_list.append({'Key': key})
+
+                    AWSRetry.exponential_backoff(
+                        catch_extra_error_codes=['InvalidSubnetID.NotFound']


This doesn't seem right to me - you may not need the catch_extra_error_codes

Ok some copy paste from other modules, but I couldn't understand purpose. I am removing it.

willthames · 2018-09-10T09:42:21Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

+        try:
+            response = self._connection.describe_internet_gateways(Filters=filters)
+            igws = response.get('InternetGateways', [])
+        except botocore.exceptions.ClientError as e:


need to catch BotoCoreError here too.

willthames · 2018-09-10T09:43:14Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

-        'changed': changed,
-        'gateway': igw_info
-    }
+    def fail(self, **kwargs):


I don't think this is really needed (and doesn't seem to be used)

willthames · 2018-09-10T09:45:19Z

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py

+                    final_tags.update(to_update)
+                else:
+                    AWSRetry.exponential_backoff(
+                        catch_extra_error_codes=['InvalidInternetID.NotFound']


InvalidInternetID doesn't seem to exist according to botocore's source

Removed line

…ed fail method

ansibot · 2018-09-10T11:28:35Z

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

lib/ansible/modules/cloud/amazon/ec2_vpc_igw.py:255:1: E302 expected 2 blank lines, found 1

click here for bot help

zeenlym · 2018-09-10T13:20:26Z

@willthames Do I need to remove classes ?

willthames · 2018-09-10T23:21:02Z

@zeenlym no, you don't need to. It has been our preference to not use classes where not required, but the classes might be useful in future anyway.

Once there is a test suite, this should be good to merge.

zeenlym · 2018-09-11T13:45:39Z

Why is there no helper to update of aws resource tags ? The only changes are client, resource-id and resource-type ?

willthames · 2018-09-12T00:21:29Z

@zeenlym, the reason why there is no update tag helper is that each AWS resource type implements their tag management API differently. Other than that, the reason is that no-one has written one.

zeenlym · 2018-09-20T07:42:56Z

Thanks for all checks, for the next 2 weeks I cannot fix nether add integration tests, I will do it after.

willthames · 2018-09-20T09:56:14Z

I've created a test suite in #45903 (based almost entirely on the egress_igw tests). I ran it before this change, then re-ran it with this change, and it's all good.

The test suite can be merged separately.

willthames · 2018-09-20T09:56:50Z

Thanks so much for the boto3 migration @zeenlym, what a great first contribution!

Yanis LISIMA added 2 commits September 5, 2018 11:40

ec2_vpc_igw: draft migrate to boto3

4921b79

ec2_vpc_igw: migrate to boto3

ba1935c

zeenlym changed the title ~~ec2_vpc_igw: draft migrate to boto3~~ ec2_vpc_igw: migrate to boto3 Sep 7, 2018

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

s-hertel approved these changes Sep 7, 2018

View reviewed changes

ec2_vpc_igw: fix error and implement code review

868e3a7

ansibot added committer_review In order to be merged, this PR must follow the certified review workflow. and 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 Sep 10, 2018

willthames suggested changes Sep 10, 2018

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed committer_review In order to be merged, this PR must follow the certified review workflow. labels Sep 10, 2018

ec2_vpc_igw: always catch BotoCoreError with ClientError, remove unus…

b29618b

…ed fail method

ansibot added the ci_verified Changes made in this PR are causing tests to fail. label Sep 10, 2018

ec2_vpc_igw: fix pep error

0f85d07

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 10, 2018

ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Sep 20, 2018

willthames approved these changes Sep 20, 2018

View reviewed changes

willthames mentioned this pull request Sep 20, 2018

Add test suite for ec2_vpc_igw before boto3 upgrade #45903

Merged

willthames merged commit 45c7fac into ansible:devel Sep 20, 2018

healem mentioned this pull request Apr 19, 2019

porting ec2_vpc_igw module from boto to boto3 #35671

Closed

ansible locked and limited conversation to collaborators Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ec2_vpc_igw: migrate to boto3 #45346

ec2_vpc_igw: migrate to boto3 #45346

zeenlym commented Sep 7, 2018

ansibot commented Sep 7, 2018

ansibot commented Sep 7, 2018

ansibot commented Sep 7, 2018

s-hertel left a comment

s-hertel Sep 7, 2018

zeenlym Sep 10, 2018

s-hertel Sep 7, 2018

zeenlym Sep 10, 2018

s-hertel Sep 7, 2018

zeenlym Sep 10, 2018

s-hertel Sep 7, 2018

zeenlym Sep 10, 2018

s-hertel Sep 7, 2018

zeenlym Sep 10, 2018

s-hertel Sep 7, 2018

zeenlym Sep 10, 2018

willthames commented Sep 10, 2018

willthames left a comment

willthames Sep 10, 2018

zeenlym Sep 10, 2018

willthames Sep 10, 2018

zeenlym Sep 10, 2018

willthames Sep 10, 2018

zeenlym Sep 10, 2018

willthames Sep 10, 2018

zeenlym Sep 10, 2018

ansibot commented Sep 10, 2018

zeenlym commented Sep 10, 2018

willthames commented Sep 10, 2018

zeenlym commented Sep 11, 2018

willthames commented Sep 12, 2018

zeenlym commented Sep 20, 2018

willthames commented Sep 20, 2018

willthames commented Sep 20, 2018

ec2_vpc_igw: migrate to boto3 #45346

ec2_vpc_igw: migrate to boto3 #45346

Conversation

zeenlym commented Sep 7, 2018

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

ansibot commented Sep 7, 2018

ansibot commented Sep 7, 2018

ansibot commented Sep 7, 2018

s-hertel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willthames commented Sep 10, 2018

willthames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansibot commented Sep 10, 2018

zeenlym commented Sep 10, 2018

willthames commented Sep 10, 2018

zeenlym commented Sep 11, 2018

willthames commented Sep 12, 2018

zeenlym commented Sep 20, 2018

willthames commented Sep 20, 2018

willthames commented Sep 20, 2018