Skip to content
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] Meraki - Initial proposal for new parameter option for response format #53891

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
3 participants
@kbreit
Copy link
Contributor

kbreit commented Mar 16, 2019

SUMMARY

Current Meraki modules respond with camelCase and not Ansible's snake_case format. This proposal is one option of how to fix this.

  • output_version parameter dictates the response key case
  • new is snake_case, old` is camelCase
  • If new, conversion is done at the end of module execution
  • This is purely a proposal and not a final draft

I am hoping this can be integrated in Ansible 2.9. This would mean the parameter would be in operation until Ansible 2.12 for a 4 year change schedule. Currently this PR defaults to new so it would break existing playbooks which use the keys. However, it could easily be changed to old for the 2.9 release.

@felixfontein I'm mrproper from IRC. This is what we were discussing earlier. I thought of this idea and wanted to get your feedback on it. I am open to doing an update() on data to add both camelCase and snake_case keys, but that would be a lot of duplicate data and verbose. I'm not saying this is the best option either, but wanted to put the idea out there.

Fixes #53598

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

meraki

Initial proposal for new parameter option for response format
- output_version parameter dictates the response key case
- new is snake_case, old is camelCase
- If new, conversion is done at the end of module execution
- This is purely a proposal and not a final draft
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 16, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 16, 2019

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

lib/ansible/module_utils/network/meraki/meraki.py:138:26: bad-whitespace No space allowed after bracket                     new = { self.key_map[k]: data[k] }                           ^
lib/ansible/module_utils/network/meraki/meraki.py:138:53: bad-whitespace No space allowed before bracket                     new = { self.key_map[k]: data[k] }                                                      ^
lib/ansible/module_utils/network/meraki/meraki.py:142:26: bad-whitespace No space allowed after bracket                     new = { snake_k: data[k] }                           ^
lib/ansible/module_utils/network/meraki/meraki.py:142:45: bad-whitespace No space allowed before bracket                     new = { snake_k: data[k] }                                              ^

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

lib/ansible/module_utils/network/meraki/meraki.py:138:28: E201 whitespace after '{'
lib/ansible/module_utils/network/meraki/meraki.py:138:53: E202 whitespace before '}'
lib/ansible/module_utils/network/meraki/meraki.py:142:28: E201 whitespace after '{'
lib/ansible/module_utils/network/meraki/meraki.py:142:45: E202 whitespace before '}'

The test ansible-test sanity --test validate-modules [explain] failed with 39 errors:

lib/ansible/modules/network/meraki/meraki_admin.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_admin.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_admin.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_config_template.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_config_template.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_config_template.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_content_filtering.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_content_filtering.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_content_filtering.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_device.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_device.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_device.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_mr_l3_firewall.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_mr_l3_firewall.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_mr_l3_firewall.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_mx_l3_firewall.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_mx_l3_firewall.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_mx_l3_firewall.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_network.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_network.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_network.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_organization.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_organization.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_organization.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_snmp.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_snmp.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_snmp.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_ssid.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_ssid.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_ssid.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_switchport.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_switchport.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_switchport.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_syslog.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_syslog.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_syslog.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])
lib/ansible/modules/network/meraki/meraki_vlan.py:0:0: E322 Argument 'output_version' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/network/meraki/meraki_vlan.py:0:0: E324 Argument 'output_version' in argument_spec defines default as ('new') but documentation defines default as (None)
lib/ansible/modules/network/meraki/meraki_vlan.py:0:0: E326 Argument 'output_version' in argument_spec defines choices as (['old', 'new']) but documentation defines choices as ([])

click here for bot help

@felixfontein
Copy link
Contributor

felixfontein left a comment

I guessed yesterday that @kbreit == mrproper ;-) (or remembered that from somewhen earlier)

This is also a way to do it. What you could do is not specify a default for output_version (except in the description), and add code which prints out a deprecation warning in case it is not specified by the user that the current default is old, but it will change to new in Ansible 2.12. Then people can stick to the old names if they want (and get rid of the warning) if they explicitly set the value to old (or whatever it will be), and already now request the new format if they explicitly specify new.

@@ -45,6 +46,7 @@ def meraki_argument_spec():
timeout=dict(type='int', default=30),
org_name=dict(type='str', aliases=['organization']),
org_id=dict(type='str'),
output_version=dict(type='str', choices=['old', 'new'], default='new'),

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

It's probably better to use more specific names than old and new, in case you want to add something else in the future. Using new2 or newer will look really strange ;)

This comment has been minimized.

@kbreit

kbreit Mar 16, 2019

Author Contributor

Old, new, newer, newest, newestAF...I dont see the big deal :)

Any ideas on replacements? I commented below with the possibility of snake and camel. But those aren’t obvious.

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

That's a very good question. Maybe an integer? Version 1 is the old format, version 2 the new. Not a very helpful notation, though. And specifying a long string is also annoying.

items[snake_k] = self.sanitize_keys(data[k])
return items
elif isinstance(data, list):
items = []

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

Why not use a list comprehension here? It's probably more efficient:
return [self.sanitize_keys(i) for i in data]

This comment has been minimized.

@kbreit

kbreit Mar 16, 2019

Author Contributor

Because I’ve never used them. But I’ll research them a bit tonight.

new = { snake_k: data[k] }
items[snake_k] = self.sanitize_keys(data[k])
return items
elif isinstance(data, list):

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

Instead of list, use Sequence. Similarly, replace dict above by Mapping. (You'll need from ansible.module_utils.common._collections_compat import Mapping, Sequence to be able to do that.)

if isinstance(data, dict):
items = {}
for k, v in data.items():
try:

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

Why not something like:

  new_key = self.key_map.get(k)
  if new_key is None:
    new_key = re.sub('([a-z0-9])([A-Z])', r'\1_\2', k).lower()
  items[new_key] = self.sanitize_keys(v)

(instead of the whole try/except block)

items = {}
for k, v in data.items():
try:
new = { self.key_map[k]: data[k] }

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

I don't think you need new anywhere.

for k, v in data.items():
try:
new = { self.key_map[k]: data[k] }
items[self.key_map[k]] = self.sanitize_keys(data[k])

This comment has been minimized.

@felixfontein

felixfontein Mar 16, 2019

Contributor

data[k] is the same as v:

Suggested change
items[self.key_map[k]] = self.sanitize_keys(data[k])
items[self.key_map[k]] = self.sanitize_keys(v)

@ansibot ansibot removed the needs_triage label Mar 16, 2019

@kbreit

This comment has been minimized.

Copy link
Contributor Author

kbreit commented Mar 16, 2019

@felixfontein My goal is to deprecate the camel case keys in Ansible 2.12 and get rid of the parameter altogether (or at least have it do nothing to avoid breaking playbooks, TBD). I’ve been trying to think of a friendly and graceful way to make this change. Here’s my current plan, but looking for feedback.

Ansible 2.9 - Introduce output_format and remove the default. However, it does stay with the snake case format if they don’t specify. I agree old and new isn’t optimal. Maybe snake and camel?
Ansible 2.10 - Introduce a warning if they don’t specify their format. No behavior change.
Ansible 2.11 - I’m thinking of changing the implicit default format to snake case in this version. But it does require playbook updates in two versions. First, to either override output_format or change playbook keys. Second, to remove output_format from playbooks in 2.12.
Ansible 2.12 - Remove camel case altogether. I may keep the output_format parameter and include a warning. Or just remove it and add a warning in 2.11.

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Mar 16, 2019

I agree that this is kind of annoying. Hmm. It's not exactly an easy problem :) Maybe just returning both keys for 2.8 - 2.11 and dropping the camel case for 2.12 is the best solution, even though until them the output is somewhat cluttered? Or just keep camel case?

@dagwieers do you have any ideas/comments/...?

@kbreit

This comment has been minimized.

Copy link
Contributor Author

kbreit commented Mar 17, 2019

@felixfontein I'm also trying to think of the proper way to merge the data structures. It's probably relatively straight forward if it's just a dictionary. I need to experiment to see if I need to do a recursive function to merge or if a singleupdate() would work. But if there's a list, which there is most of the time, it's more complicated.

I can't guarantee the top level key is going to be different. It could be, lets say network so it wouldn't have any conversion. If the top level structure is a list, which it often is, I can't merge them so the list instead of 5 items long is 10. That would break data structures relating to things like firewall rules.

I'm sure there's one or two other ways I could merge them into a single data structure. If you have better ideas, I'm all ears (or eyes. we're reading).

@felixfontein

This comment has been minimized.

Copy link
Contributor

felixfontein commented Mar 17, 2019

Since different keys can show up at all levels (but do not have to), you'd have to do a recursive merge. But that shouldn't be that much harder than your current sanitize_keys function (in fact, you need that one as well, to be called in some situations).

If there's a list (whose key is both snake and camel case), it's not a problem, you'll keep the list, but call the merge function for all its entries (depending on what they are). If the entries are neither lists nor dicts, nothing needs to be changed. If they are lists or dicts, you can do the recursive merge for these entries.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.