Refactor ec2_vpc_dhcp_option to use boto3 (breaking change) #252

jillr · 2021-02-01T20:16:04Z

SUMMARY

Convert to boto3
More consistent use of check_mode
Use AwsRetry
Catch BotoCoreError, ClientError

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

plugins/modules/ec2_vpc_dhcp_option.py

ansibullbot · 2021-02-01T20:19:05Z

cc @joelthompson @s-hertel @tremble @wimnat
click here for bot help

plugins/modules/ec2_vpc_dhcp_option.py

jillr · 2021-02-09T21:39:27Z

@tremble What would you think about the boto3 version of this module having occasionally slightly different return data, but substantially more consistent (and documented) returns? Things like, in the boto2 module we sometimes returned all DhcpConfigurations keys, we sometimes only returned the ones that were set, and sometimes (such as the - name: add and remove tags test) returned no keys. I'm thinking about always returning all keys, with None set if there's no value, in the interest of simplifying the internal logic.
ETA: although if I'm not trying to sometimes return all keys, it should be easier to always only return keys that are set.

Whether something is returned as a string or a list should also be more predictable now, though I haven't modified the integration tests to confirm for certain yet.
(All the places where we currently have things like dhcp_options.new_options['domain-name'] in ["{{ aws_domain_name }}", ["{{ aws_domain_name }}"]] right now.)

Basically this has turned into a full rewrite, so I'm considering making it part of a 2.0 of the collection and being a bit freer with making the code long-term easier to maintain over a perfect 1:1 matching of the boto2 module.

tremble · 2021-02-10T10:52:03Z

I'm generally good with returning keys where we didn't before.

Changing what existing return values return (strings vs lists) makes me less comfortable and might be better handled with a breaking release. It's really hard to communicate this to users and 'deprecate' return values.

In general I'd like to be very deliberate about performing a 2.x release since it becomes the point at which we'll probably get asked to start backporting fixes between branches.

Are there other places where we'd like to 'break' return values for consistency?
- Do we have anywhere other than ec2_vpc_igw_info still returning tags as a list of dicts rather than a simple list?
- Do we want to remove all the old boto2 compatibility return hacks?
- De we want to push towards more consistent return values, maybe adding the new return types in 2.x and removing the old ones in a 3.x
Would a 2.x release be a good time to nuke all the _facts aliases?
Would a 2.x release be a good time to make module names a little more consistent? (aws_ is a little redundant and we have a lot of ec2_ modules people might not realise are tied to the EC2 API)

jillr · 2021-02-10T16:40:24Z

A 2.0 would definitely be more deliberate, I'd see that as happening at the same time we move the first batch of modules out of community.aws and 2.0 that collection (hopefully later this spring). That would leave this PR WIP until that time, but free me up to simplify the logic quite a bit.

We put the facts aliases deprecation notices as 2021-12-01, so I think we're best off waiting until after then regardless of collection version.

I would be in favor of starting to update any return changes and module names that we can in 2.x.

jillr · 2021-02-17T00:57:42Z

This is mostly done, minus unit tests. There are some lingering bugs noted in the integration test file but I'm inclined to land this already extensive rewrite and handle those later as they're are for features or functions that the module has never provided anyway.

ansibullbot · 2021-02-17T01:00:15Z

cc @naslanidis
click here for bot help

tremble

A couple of niggles, nothing major jumping out at me

plugins/modules/ec2_vpc_dhcp_option.py

tremble · 2021-02-17T11:20:10Z

tests/integration/targets/ec2_vpc_dhcp_option/tasks/main.yml

  - assert:
      that:
        - dhcp_options.changed
        - dhcp_options.new_options
        # FIXME extra keys are returned unpredictably
-        - dhcp_options.new_options.keys() | list | sort is superset(['netbios-name-servers', 'netbios-node-type', 'ntp-servers'])
+        - dhcp_options.new_options.keys() | list | sort == ['netbios-name-servers', 'netbios-node-type', 'ntp-servers']


Suggested change

- dhcp_options.new_options.keys() | list | sort == ['netbios-name-servers', 'netbios-node-type', 'ntp-servers']

- dhcp_options.new_options.keys() | list | sort == ['netbios-name-servers', 'netbios-node-type', 'ntp-servers']

I was thinking that maybe failing if a new option was added is the best option here. However, I think if someone wants to add additional info, we should probably get them to return the boto3 style data alongside the original boto2 data.

Maybe worth a comment here about why we fail if extra return values are passed?

Sorry, I may just be being dense but I'm not sure what you're asking on this. We should only have these 3 keys here because those should be the only 3 keys that are configured in this option set's DhcpConfiguration at this point.

Are you asking if we should return the normalized_config() (which attempts to preserve new_config from the original module insofar as new_config wasn't even entirely consistent in the first place) as well as just dumping the boto3 response?

We should only have these 3 keys here because those should be the only 3 keys that are configured in this option set's DhcpConfiguration at this point.

Ah, ok so it's supposed to have done something approximating purge_none_params

Most of the time we do - '"foo" in bar.baz' to say we only care that the mentioned keys are there, not that it must be only specific keys (it gives us flexibility if Amazon add extra features). I'm worried about non-standard examples knocking about that'll confuse people. I'd be good with just a comment that "this is non-standard behaviour because..."

Are you asking if we should return the normalized_config() (which attempts to preserve new_config from the original module insofar as new_config wasn't even entirely consistent in the first place) as well as just dumping the boto3 response?

In general, Yes.

Gotcha. Added a quick take at that, no tests yet (in the current incarnation it would solve the need to return tags at least). I really don't like the whole Key, Values, Value structure that amazon is giving us here but that's what we have to work with.

jillr · 2021-02-22T22:45:58Z

@tremble Took a swipe at returning both result formats in both modules as discussed.

markuman · 2021-03-01T21:16:41Z

plugins/module_utils/ec2.py

+            {'Key': 'domain-name-servers', 'Values': [{'Value': 'AmazonProvidedDNS'}]},
+            {'Key': 'netbios-name-servers', 'Values': [{'Value': '1.2.3.4'}, {'Value': '5.6.7.8'}]},
+            {'Key': 'netbios-node-type', 'Values': [1]},
+             {'Key': 'ntp-servers', 'Values': [{'Value': '1.2.3.4'}, {'Value': '5.6.7.8'}]}


one whitespace too much , but not worth an extra commit

markuman · 2021-03-01T21:17:42Z

plugins/module_utils/ec2.py

+        return config_data
+
+    for config_item in option_config:
+        # # Handle single value keys


one # too much , but not worth an extra commit

markuman · 2021-03-01T21:23:44Z

plugins/modules/ec2_vpc_dhcp_option.py

+    if purge_tags and not tags:
+        tags_to_unset = current_tags
+
+    if tags_to_unset:


as fas as I see if tags_to_unset: is not necessary here. The logic can instantly applied after L276 if purge_tags and not tags
but on the other hand, the code logic looks cleaner and more separated.

My concern is keeping things separated enough for the changed logic to be clear. I'm not sure where I would like to put the changed=True to cover a case where there are no tags_to_set but there are tags_to_unset without potentially setting the wrong changed in other cases. It might be a bit verbose this way but it's consistent with what we do in other modules, my inclination would be to leave it a bit more easy to read.

markuman · 2021-03-01T21:30:45Z

plugins/modules/ec2_vpc_dhcp_option.py

+    params = module.params
+    if params['domain_name'] is not None:
+        new_config.append({'Key': 'domain-name', 'Values': [{'Value': params['domain_name']}]})
+    if params['dns_servers'] is not None:


what about just params.get('domain_name'): and reduce if and forin single for server in params.get('dns_servers', []):
but yeah, then the extra vars (dns_server_list, ntp_server_list etc are always be initialized, even if not used.

Historically the default value of those options is None, not an empty list, and we can't iterate on None. If I change the default to [] I get cascading effects in the data transformations where null values get set on options that should be unset, which breaks changed status assertions. It should be possible to make this change but it would require changing code in several places unfortunately.

markuman

LGTM

tremble

Minor tweaks to changelog and thoughts about what's returned.

changelogs/fragments/252_boto3_refactor_ec2_vpc_dhcp_option.yaml

plugins/modules/ec2_vpc_dhcp_option.py

tremble

whitespace fixup

plugins/modules/ec2_vpc_dhcp_option_info.py

* Convert to boto3 * More consistent use of check_mode * Use AwsRetry * Catch BotoCoreError, ClientError

…g and the user-friendly "normalized" comnfig. Move the normalize_config function to module_utils Enable integration test assertions for new return values, including tags

jillr · 2021-04-29T18:33:29Z

rebased around the git conflict.

ansibullbot added WIP Work in progress feature This issue/PR relates to a feature request module module needs_triage owner_pr PR created by owner/maintainer plugins plugin (any type) labels Feb 1, 2021

goneri reviewed Feb 1, 2021

View reviewed changes

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

goneri removed the needs_triage label Feb 3, 2021

ansibullbot added integration tests/integration tests tests and removed owner_pr PR created by owner/maintainer labels Feb 6, 2021

tremble requested changes Feb 17, 2021

View reviewed changes

jillr force-pushed the ec2_vpc_dhcp_boto3 branch from b7bcb4c to 6d1f532 Compare February 18, 2021 00:13

ansibullbot added needs_maintainer new_plugin New plugin labels Feb 18, 2021

ansibullbot added the module_utils module_utils label Feb 22, 2021

jillr changed the title ~~[WIP] Refactor ec2_vpc_dhcp_option to use boto3~~ Refactor ec2_vpc_dhcp_option to use boto3 Feb 23, 2021

jillr removed the WIP Work in progress label Feb 23, 2021

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Feb 23, 2021

markuman reviewed Mar 1, 2021

View reviewed changes

markuman approved these changes Mar 4, 2021

View reviewed changes

tremble requested changes Mar 4, 2021

View reviewed changes

changelogs/fragments/252_boto3_refactor_ec2_vpc_dhcp_option.yaml Outdated Show resolved Hide resolved

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

jillr force-pushed the ec2_vpc_dhcp_boto3 branch 3 times, most recently from 75ce5b4 to 48ae8da Compare March 8, 2021 18:27

tremble approved these changes Mar 10, 2021

View reviewed changes

tremble changed the title ~~Refactor ec2_vpc_dhcp_option to use boto3~~ Refactor ec2_vpc_dhcp_option to use boto3 (breaking change) Mar 11, 2021

tremble reviewed Mar 17, 2021

View reviewed changes

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

tremble reviewed Mar 17, 2021

View reviewed changes

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

jillr and others added 13 commits April 29, 2021 11:24

Refactor ec2_vpc_dhcp_option to use boto3

9fc52ad

* Convert to boto3 * More consistent use of check_mode * Use AwsRetry * Catch BotoCoreError, ClientError

Ongoing refactoring

a0f2d9b

Ongoing rewrite, start updating tests

b8dcd5f

refactoring largely complete, fix tag return in ec2_vpc_dhcp_option_info

b4b5a55

Add unit tests, cleanup some quotes in module and tests

09a684c

Return boto3-style result as well

ad3cdd7

sanity fix

ee7fe47

Make the module and info module return both the boto-structured confi…

bc78869

…g and the user-friendly "normalized" comnfig. Move the normalize_config function to module_utils Enable integration test assertions for new return values, including tags

address review feedback

48d2714

Return dhcp_config from ec2_vpc_dhcp_option and deprecate new_config.

95e62ac

whitespace sanity fix

a5f2d3e

Add ignore for pylint:ansible-deprecated-no-version (ansible-2.9)

a3cd932

remove duplicate contains

4f49c39

jillr force-pushed the ec2_vpc_dhcp_boto3 branch from d1f856d to 4f49c39 Compare April 29, 2021 18:32

ansibullbot added community_review and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 29, 2021

jillr added the gate label Apr 29, 2021

ansible-zuul bot merged commit 762674e into ansible-collections:main Apr 29, 2021

jillr deleted the ec2_vpc_dhcp_boto3 branch July 2, 2021 22:47

tremble mentioned this pull request Oct 18, 2022

[6.0.0] ec2_vpc_dhcp_option - drop new_config return key #1177

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ec2_vpc_dhcp_option to use boto3 (breaking change) #252

Refactor ec2_vpc_dhcp_option to use boto3 (breaking change) #252

jillr commented Feb 1, 2021

ansibullbot commented Feb 1, 2021

jillr commented Feb 9, 2021 •

edited

Loading

tremble commented Feb 10, 2021

jillr commented Feb 10, 2021

jillr commented Feb 17, 2021

ansibullbot commented Feb 17, 2021

tremble left a comment

tremble Feb 17, 2021

jillr Feb 18, 2021

tremble Feb 18, 2021

jillr Feb 18, 2021

jillr commented Feb 22, 2021

markuman Mar 1, 2021

markuman Mar 1, 2021

markuman Mar 1, 2021

jillr Mar 3, 2021

markuman Mar 1, 2021 •

edited

Loading

jillr Mar 3, 2021

markuman left a comment

tremble left a comment

tremble left a comment

jillr commented Apr 29, 2021

	- dhcp_options.new_options.keys() \| list \| sort == ['netbios-name-servers', 'netbios-node-type', 'ntp-servers']
	- dhcp_options.new_options.keys() \| list \| sort == ['netbios-name-servers', 'netbios-node-type', 'ntp-servers']

Refactor ec2_vpc_dhcp_option to use boto3 (breaking change) #252

Refactor ec2_vpc_dhcp_option to use boto3 (breaking change) #252

Conversation

jillr commented Feb 1, 2021

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Feb 1, 2021

jillr commented Feb 9, 2021 • edited Loading

tremble commented Feb 10, 2021

jillr commented Feb 10, 2021

jillr commented Feb 17, 2021

ansibullbot commented Feb 17, 2021

tremble 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

jillr commented Feb 22, 2021

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

markuman Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markuman left a comment

Choose a reason for hiding this comment

tremble left a comment

Choose a reason for hiding this comment

tremble left a comment

Choose a reason for hiding this comment

jillr commented Apr 29, 2021

jillr commented Feb 9, 2021 •

edited

Loading

markuman Mar 1, 2021 •

edited

Loading