New module: manage Amazon CloudFront origin access identities (cloud/amazon/cloudfront_origin_access_identity) #24568

wilvk · 2017-05-12T22:13:45Z

SUMMARY

As advised by @ryansb in PR #24292 this comprises the origin access identity component of the cloudfront module additions.

ISSUE TYPE

New Module Pull Request

COMPONENT NAME

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

ANSIBLE VERSION

  config file =
  configured module search path = [u'/Users/willvk/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/willvk/Source/gits/ansibles/ansible/lib/ansible
  executable location = /Users/willvk/Source/gits/ansibles/ansible/bin/ansible
  python version = 2.7.10 (default, Feb  6 2017, 23:53:20) [GCC 4.2.1 Compatible Apple LLVM 8.0.0 (clang-800.0.34)]

ADDITIONAL INFORMATION

ryansb · 2017-05-16T14:29:55Z

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

+    argument_spec.update(dict(
+        state=dict(choices=['present', 'updated', 'absent'], default='present'),
+        origin_access_identity_id=dict(required=True, default=None, type='str'),
+        caller_reference=dict(required=False, default=None, type='str'),


This parameter isn't documented.

added to doc

ryansb · 2017-05-16T14:30:43Z

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

+
+from ansible.module_utils.ec2 import get_aws_connection_info, ec2_argument_spec
+from ansible.module_utils.ec2 import boto3_conn, HAS_BOTO3
+from ansible.modules.cloud.amazon.cloudfront_distribution import CloudFrontHelpers


To share code between modules, please use module_utils instead of importing other modules directly.

ryansb · 2017-05-16T14:33:05Z

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

+    def delete_origin_access_identity(self, origin_access_identity_id, e_tag):
+        try:
+            func = partial(self.client.delete_cloud_front_origin_access_identity,
+                           Id=origin_access_identity_id, IfMatch=e_tag)


This isn't ever going to be paginated. docs

no - removed

ryansb · 2017-05-16T14:33:41Z

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

+                               'CallerReference': caller_reference,
+                               'Comment': comment
+                           })
+            return self.paginated_response(func)


Create/update/delete calls aren't paginated in boto3. Are you using this pagination function for other error handling as well?

removed paginated - was only using to pop ResponseMetadata which have added back

ryansb · 2017-05-16T14:34:18Z

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

+                               'Comment': comment
+                           },
+                           Id=origin_access_identity_id, IfMatch=e_tag)
+            return self.paginated_response(func)


ryansb · 2017-05-18T17:14:29Z

lib/ansible/module_utils/cloudfront.py

+#   - cloudfront_origin_access_identity
+
+
+class CloudFrontHelpers:


This really doesn't need to be an object - I'd prefer to have from module_utils.cloudfront import helpers to instantiating a class.

ryansb · 2017-05-18T17:15:28Z

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

+    pass
+
+
+class CloudFrontOriginAccessIdentityServiceManager:


This needs to be class CloudFrontOriginAccessIdentityServiceManager(object): to use new-style 2.7 classes.

ansibot · 2017-07-18T22:25:04Z

cc @willthames
click here for bot help

willthames · 2017-11-20T00:20:41Z

With cloudfront_distribution I updated it to just create an origin access identity when needed - do you think there's a need for a separate management of OAI?

wilvk · 2017-11-20T11:20:51Z

@willthames probably not unless there is a compelling use case for it.

s-hertel · 2018-01-18T18:59:21Z

lib/ansible/module_utils/cloudfront.py

+    return dictionary
+
+
+def snake_dict_to_pascal_dict(snake_dict):


snake_dict_to_camel_dict now takes the parameter capitalize_first and camel_dict_to_snake_dict takes reversible so those can be used instead these pascal functions.

this file is no longer required as it is now in ansible/module_utils/ec2.py

i might just open a new PR for this and only keep the origin_access_identity file as the rebase is going to be a nightmare

Sure, whatever is easiest. I'm sorry about the rebase nightmare.

s-hertel · 2018-01-18T19:00:15Z

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

@@ -238,8 +238,8 @@
    pass  # will be caught by imported HAS_BOTO3


-class CloudFrontServiceManager:
-    """Handles CloudFront Services"""
+class CloudFrontFactsServiceManager:


This should be a new style class CloudFrontFactsServiceManager(object)

s-hertel · 2018-01-18T19:01:51Z

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

+#!/usr/bin/python
+# This file is part of Ansible
+#
+# Ansible is free software: you can redistribute it and/or modify


The copyright can be updated: http://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#copyright

s-hertel · 2018-01-18T19:05:01Z

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

+    CloudFrontFactsServiceManager)
+import ansible.module_utils.cloudfront as helpers
+from ansible.module_utils.ec2 import camel_dict_to_snake_dict
+from ansible.module_utils.basic import AnsibleModule


It would be better if AnsibleAWSModule was used instead of AnsibleModule because the exception handling is better (and cloudfront_distribution is using it). To import: from ansible.module_utils.aws.core import AnsibleAWSModule

Instead of using e.response and str(e) yourself, you can call

except (ClientError, BotoCoreError) as e: module.fail_json_aws(e, msg="Unable to <thing that failed>")

and it formats the exception by using e.message if it exists or str(e) and uses e.response only if it exists.

Using AnsibleAWSModule also means you can remove importing HAS_BOTO3 and the HAS_BOTO3 check, as the module checks it.

s-hertel · 2018-01-18T19:13:54Z

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

+            self.client = boto3_conn(
+                self.module, conn_type='client', resource=resource,
+                region=region, endpoint=ec2_url, **aws_connect_kwargs)
+        except botocore.exceptions.NoRegionError:


boto3_conn() now handles NoRegionError and ClientError so you can remove that here.

s-hertel · 2018-01-18T19:15:55Z

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

+                    'CallerReference': caller_reference,
+                    'Comment': comment
+                })
+        except botocore.exceptions.ClientError as e:


BotoCoreError should be caught here as well and all other places catching ClientError https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/cloud/amazon/GUIDELINES.md#boto3-1

BotoCoreError does not have a .response, so using AnsibleAWSModule will cut down on exception handling logic as I suggested above.

s-hertel · 2018-01-18T19:18:10Z

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

+    argument_spec.update(dict(
+        state=dict(choices=['present', 'absent'], default='present'),
+        origin_access_identity_id=dict(required=False, default=None,
+                                       type='str'),


type='str' is the default so you can remove that. default=None and required=False are also defaults that can be removed.

s-hertel · 2018-01-18T19:21:06Z

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

+        else:
+            result = service_mgr.create_origin_access_identity(
+                caller_reference, comment)
+        changed = True


It looks like this module is not idempotent. Running the same task twice with state: present or state: absent will show changed every time.

s-hertel · 2018-01-18T19:23:42Z

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

+            origin_access_identity_id, e_tag)
+        changed = True
+
+    if result:


I think you can just do result.pop('ResponseMetadata', None) and then call module.exit_json with result

s-hertel · 2018-01-18T19:28:04Z

@wilvk These requested changes should also be applied to #24567.

…w ok instead of changed when deleting same oai multiple times

…so made always present for add and update

wilvk · 2018-01-31T11:24:11Z

Closing as have migrated changes to new PR #35540

bcoca removed the needs_triage Needs a first human triage before being processed. label May 15, 2017

ryansb suggested changes May 16, 2017

View reviewed changes

ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. c:module_utils/ and removed community_review In order to be merged, this PR must follow the community review workflow. labels May 16, 2017

ryansb suggested changes May 18, 2017

View reviewed changes

alikins changed the title ~~cloudfront_origin_access_identity.py~~ New module: Add cloudfront_origin_access_identity (cloud/amazon/cloudfront_origin_access_identity) May 22, 2017

alikins changed the title ~~New module: Add cloudfront_origin_access_identity (cloud/amazon/cloudfront_origin_access_identity)~~ New module: manage Amazon CloudFront origin access identities (cloud/amazon/cloudfront_origin_access_identity) Jun 5, 2017

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. support:community This issue/PR relates to code supported by the Ansible community. labels Jun 23, 2017

ansibot added the stale_review Updates were made after the last review and the last review is more than 7 days old. label Jul 3, 2017

ansibot added the module_utils/ label Sep 1, 2017

wilvk mentioned this pull request Nov 19, 2017

Cloudfront distribution #31284

Merged

ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Nov 20, 2017

s-hertel requested changes Jan 18, 2018

View reviewed changes

wilvk force-pushed the cloudfront_origin_access_identity branch from 11540fc to 743c116 Compare January 20, 2018 07:23

ansibot removed 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 Jan 20, 2018

wilvk added 21 commits January 28, 2018 10:53

doc fix

a43f2a2

fixed default, naming of method and error message

7f8e099

invalidation paraneter validation

0c3349a

cloudfront origin access identity split to separate PR

9072d1b

refactoring as advised

5a8cbf8

added to doc, formatting

8ec9293

helpers class to methods, formatting

24144b6

merged present and updated, pep8 formatting, added changed var to sho…

416091f

…w ok instead of changed when deleting same oai multiple times

only add comment when adding and is none, otherwise, pass in none

a32b51c

refactoring out for cleaner code, comment is only changing attribute …

94a779a

…so made always present for add and update

fix for zero length arrays conversion to aws

74143f6

fix copyright

0180fa7

fix formatting on import

4e698a6

fix exception handling

f30592b

remove no region exception - now handled

223eab3

removed defaults

463b91a

neaten result pop

5a8c7a6

fix error checks

77ae49f

fix invalid refs

1e57b59

fix docs

823a56d

format docs

0dac57c

wilvk force-pushed the cloudfront_origin_access_identity branch from 1d82a78 to 0dac57c Compare January 27, 2018 23:53

ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html ci_verified Changes made in this PR are causing tests to fail. and removed ci_verified Changes made in this PR are causing tests to fail. labels Jan 27, 2018

wilvk closed this Jan 31, 2018

wilvk mentioned this pull request Jan 31, 2018

New module: manage Amazon CloudFront origin access identities (cloud/amazon/cloudfront_origin_access_identity) #35540

Merged

wilvk deleted the cloudfront_origin_access_identity branch January 31, 2018 11:27

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New module: manage Amazon CloudFront origin access identities (cloud/amazon/cloudfront_origin_access_identity) #24568

New module: manage Amazon CloudFront origin access identities (cloud/amazon/cloudfront_origin_access_identity) #24568

wilvk commented May 12, 2017

ryansb May 16, 2017

wilvk May 17, 2017

ryansb May 16, 2017

wilvk May 17, 2017

ryansb May 16, 2017

wilvk May 17, 2017

ryansb May 16, 2017

wilvk May 17, 2017

ryansb May 16, 2017

wilvk May 17, 2017

ryansb May 18, 2017

wilvk May 19, 2017

ryansb May 18, 2017

wilvk May 19, 2017

ansibot commented Jul 18, 2017

willthames commented Nov 20, 2017

wilvk commented Nov 20, 2017

s-hertel Jan 18, 2018

wilvk Jan 27, 2018

wilvk Jan 27, 2018

s-hertel Jan 29, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel Jan 18, 2018

s-hertel commented Jan 18, 2018

wilvk commented Jan 31, 2018

		# - cloudfront_origin_access_identity


		class CloudFrontHelpers:

New module: manage Amazon CloudFront origin access identities (cloud/amazon/cloudfront_origin_access_identity) #24568

New module: manage Amazon CloudFront origin access identities (cloud/amazon/cloudfront_origin_access_identity) #24568

Conversation

wilvk commented May 12, 2017

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ADDITIONAL INFORMATION

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

Choose a reason for hiding this comment

ansibot commented Jul 18, 2017

willthames commented Nov 20, 2017

wilvk commented Nov 20, 2017

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

s-hertel commented Jan 18, 2018

wilvk commented Jan 31, 2018