Convert more keys/attribute values from list to set

[dhoffend]

Related Issue

Fixes #1486

New Behavior

In addition to tags, the fields tagged_vlans and object_types should be considered a set, not an ordered list. This should be in sync with pynetbox as well. (https://github.com/netbox-community/pynetbox/blob/master/pynetbox/core/response.py#L25-L26) I'll propose that object_types will be added there as well.

This is a follow up on #1456 to take a more generic approach when lists should be converted to/compared as set

...

Contrast to Current Behavior

The field object_types is currently handled as a sorted list, but the response is unsorted resuting in constand changed tasks, even nothing has changed.

...

Discussion: Benefits and Drawbacks

I don't see any drawbacks as object_types is not considered to be ordered.

...

Changes to the Documentation

Nothing, as it doesn't affect the outcome, it just fixes a display issue (changed vs ok)

Proposed Release Note Entry

Take a more generic approach for set comparison. Other models have object_types too
...

Double Check

• I have read the comments and followed the CONTRIBUTING.md.
• I have explained my PR according to the information in the comments or in a linked issue.
• My PR targets the devel branch.

[dchiquito]

I think there's some overlap here with #1456

[sc68cal]

The sanity failures are unrelated to your patch. I think a new version of ansible's sanity test checker was released and they've been increasing the strictness. Let me investigate and see if there's an easy fix

plugins/modules/netbox_virtual_machine.py:0:0: bad-return-value-key: Return value key 'virtual machine' should not be used for return values since it cannot be accessed with dot notation in Jinja

[sc68cal]

Please rebase this on the latest commits to devel to pick up the fix, and this should be good to go. I'm 👍 on this being merged as soon as it passes CI checks

[dhoffend]

@sc68cal thank you for looking into it.

Rebase is done and pushed, but now I'm a bit more confused. After looking into the other merge request you mentioned, they're doing basically the same, but in the netbox_users.py context.

Now we've 3 positions where list-set conversions are beeing processed:

• module_utils/netbox_users.py
• module_utils/netbox_utils.py
• pynetbox/core/response.py (I've a similar merge request open to do basically the same)

Which approach should be the best?

I've the feeling that it should be some kind of attribute/parameter to the common functions. So every endpoint module can define their list of attributes that should be considered a set.

[dhoffend]

@sc68cal I've renamed the title and reworked my patch a bit. I partly reverted the previous commit in netbox_users and moved the conversion to utils. There are more modules that make use of object_classes attributes etc.

I have to do some more testing, but basically this could up for discussions.

I've added a some additional fields that probably should be compared as set (like tagged_vlans, etc)

I'm not exactly sure which attributes are considered a set, but I took a conservative approach. Maybe we should check them with multiple eyes. Things to check: config_context: (teants, sites, clusters, etcs.)

[dhoffend]

@dchiquito Maybe you can give me your thoughts as well, as it effects your previous commit as well.

[sc68cal]

@dhoffend If you have PR to pynetbox that solves this at a lower layer, that's great. If not, let's try and figure out a way forward. It may have to be that we fix individual modules as we go, until we determine a pattern that allows us to consolidate everything into a single location. Overall, I am fine with working incrementally instead of waiting for a one-size-fits-all solution.

[dchiquito]

I was only aware of the fields in the contact module when I made my changes, which is why I changed netbox_users.py instead of netbox_utils.py. You have more fields here so your implementation is more appropriate. I think my changes should be removed in this PR. My tests should still work, though.

However, the best solution would be to upstream the fix pynetbox so we don't have to add any extra code to ansible_modules.

[dhoffend]

I guess there are more set attributes in the config_context sections (where the config contexts should be applied to). I just hadn't yet the time to test things out.

Sure a bigger list of endpoints and attributes (which should be considered a set) in the underlaying pynetbox would makes sense. But I'm not sure if that's the only solution. Since we must compare the pynetbox result (which maybe "could" be returned as set or ordered lists ...) and the module user input (which would be a list), I'm not sure if I side I would trust more.

netbox-community/pynetbox#712

[dhoffend]

Hmmmm looking at pynetbox code:
https://github.com/netbox-community/pynetbox/blob/master/pynetbox/core/response.py#L527-L531

This doesn't look like a real set() type convert to me, more like a quick fix.