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

WIP: enable diff mode for AWS modules #37212

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@s-hertel
Contributor

s-hertel commented Mar 8, 2018

SUMMARY

WIP. Unit tests, integration tests, utilization to come.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

lib/ansible/module_utils/aws/core.py

ANSIBLE VERSION
2.6

@s-hertel s-hertel force-pushed the s-hertel:WIP_AWS_diff_mode branch Mar 8, 2018

@s-hertel s-hertel added cloud aws and removed needs_triage labels Mar 8, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 8, 2018

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

lib/ansible/module_utils/aws/core.py:171:28: undefined-variable Undefined variable 'string_types'
lib/ansible/module_utils/aws/core.py:175:30: undefined-variable Undefined variable 'OrderedDict'
lib/ansible/module_utils/aws/core.py:182:32: undefined-variable Undefined variable 'string_types'

click here for bot help

@ansibot ansibot added the ci_verified label Mar 8, 2018

@s-hertel s-hertel force-pushed the s-hertel:WIP_AWS_diff_mode branch to e63eaaa Mar 8, 2018

@ansibot ansibot removed the ci_verified label Mar 8, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Mar 9, 2018

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

lib/ansible/module_utils/aws/core.py:54:0: ImportError: cannot import name OrderedDict

click here for bot help

return [self.create_response_dict(i) for i in item]
# from collections import OrderedDict fails on Python 2.6
# botocore uses its own OrderedDict class that is compatible with Python 2.6
elif item.__class__.__name__ == 'OrderedDict':

This comment has been minimized.

@JordonPhillips

JordonPhillips May 14, 2018

Contributor

Why not check the actual type? In botocore we'll use the backport from the ordereddict package on 2.6. So botcore.compat.OrderedDict will be the version from collections on versions that support it. Are there other versions that you're looking to support?

@@ -159,3 +167,120 @@ def fail_json_aws(self, exception, msg=None):
else:
self._module.fail_json(msg=message, exception=last_traceback,
**camel_dict_to_snake_dict(response))
def create_response_dict(self, item):

This comment has been minimized.

@JordonPhillips

JordonPhillips May 14, 2018

Contributor

It looks like all this method is doing is un-ordering dicts, so something like convert_response may be a better name.

elif isinstance(response, list):
return [self.populate_response(i, extra_output) for i in response]
elif isinstance(response, dict):
for output_key, v in dict(response).items():

This comment has been minimized.

@JordonPhillips

JordonPhillips May 14, 2018

Contributor

You shouldn't need to cast this to a dict since you've already asserted it is one.

# Careful. If doing diffs with deeply nested structures with repeat keys this will
# do weird things. Safer to pass extra_output as a complex structure in cases like that.
# TODO: make extra_output smarter
response[output_key] = self.populate_response(v, extra_output)

This comment has been minimized.

@JordonPhillips

JordonPhillips May 14, 2018

Contributor

What's the expected structure of extra_output? It looks like you're not traversing it.

# Generate response dict
resp = self.create_response_dict(output_args)
if extra_output:
resp = self.populate_response(dict(resp), extra_output)

This comment has been minimized.

@JordonPhillips

JordonPhillips May 14, 2018

Contributor

This will fail if the response type isn't a dict. If it is a dict, you don't need to cast it since you've already done that in create_response_dict.

Also both of these methods traverse the dictionaries. That's probably not an issue, but it might makes sense to consolidate them so you're only traversing it once.

if method:
if not any(method.startswith(r) for r in ('get', 'describe', 'list')):
self.warn("This may be an implementation error of diff mode. The compare "
"method should be a read-only method. Setting 'before' to an empty dict.")

This comment has been minimized.

@JordonPhillips

JordonPhillips May 14, 2018

Contributor

Yeah, I wish we had better metadata around this. You can look at the http method, but POST might be used for read-only stuff for some services. I'm pretty sure GET methods don't modify state, but not 100%.

@ansibot ansibot added the affects_2.6 label May 22, 2018

@s-hertel s-hertel force-pushed the s-hertel:WIP_AWS_diff_mode branch from 9538545 Jun 5, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Jun 5, 2018

@s-hertel s-hertel force-pushed the s-hertel:WIP_AWS_diff_mode branch to d67a5c0 Jun 5, 2018

s-hertel added some commits Jun 4, 2018

rename method
remove redundant casting to dict

Use 2.6 compatible OrderedDict

add self._diff and self.difflist to AnsibleAWSModule
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment