DRAFT PR - Use sentinel value to allow for distinguishing between implicit and null values

[robduffy2010]

This allows use to set a field to null in Netbox using this module

Related Issue

#330

New Behavior

Setting a field to null in the Ansible Netbox module will set a null value for the same field in Netbox.

Contrast to Current Behavior

Right now, setting a field to null does nothing.

Discussion: Benefits and Drawbacks

Right now, there is no way to set a field to null using the Ansible Netbox module. This can be important for different reasons, like when a VLAN should be removed from an interface or when an asset tag needs to be removed when recycling a device.

Changes to the Documentation

Unclear. Perhaps it can be documented that it is now possible to set fields to null although it's intuitive behaviour.

Proposed Release Note Entry

Added ability to set fields to null when supported by Netbox.

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.

[sc68cal]

Using the is operator is for testing if two objects are the same in memory. I don't know if that's the correct logic.

[sc68cal]

I am going to re-run the jobs. It might have been impacted by the GH outage yesterday. Can you make a simple changelog fragment so that this can get through that test, so that it does a full CI run? Content doesn't matter, just enough to make the job happy.

[robduffy2010]

Sure, I'll do that shortly. I'm thinking now that the OMITTED object should be instantiated in one file and then imported into others from there, so I'll make that change.

[sc68cal]

OK. I like where this is going, but I have some thoughts:

Because you define OMITTED as just object - here's what some errors are like.

Argument 'virtual_chassis' in argument_spec found in data defines default as (<object object at 0x7f187cd38950>) but documentation defines default as (None)

So it got me thinking. What if we instead did something like:

class OmittedArgument():

    def __str__(self):
        return "Omitted Argument"

First off, that makes it much better for debugging purposes since it's not just printing object and an address in memory.

Secondly, it allows us to check based on it being an instance of the class, rather than relying on it having the same memory location. I just get concerned that it could somehow bite us.

Which means some of your logic would change to:

            elif v not isinstance(OmmittedArgument):
                ...

Thoughts?

[robduffy2010]

I really much prefer your suggestion actually. I went ahead and updated the PR. If you're happy with it, I can make the changes for the rest of the modules.

[sc68cal]

Let's keep iterating on it here. I like the idea so far, I think the next thing is maybe build a test case for creating a device in a rack, then un-racking it via the OmittedArgument since that's a common ask.

I'm okay with hacking it to make it work since we're still at the "would this work like we think phase" so feel free to focus on expediency rather than correctness. I also have a TODO for myself to figure out why the sanity checks are failing, sometimes that's an upstream change that bites us but it's sometimes hard to determine.

Great work so far, thank you for contributing.

[sc68cal]

ERROR: plugins/modules/netbox_device.py:0:0: doc-choices-do-not-match-spec: Argument 'airflow' in argument_spec found in data defines choices as (['front-to-rear', 'rear-to-front', 'left-to-right', 'right-to-left', 'side-to-rear', 'passive', 'mixed', <ansible_collections.netbox.netbox.plugins.module_utils.netbox_utils.OmittedArgument object at 0x7f4ef01f0390>]) but documentation defines choices as (['front-to-rear', 'rear-to-front', 'left-to-right', 'right-to-left', 'side-to-rear', 'passive', 'mixed'])

Ok looks like there might be another property or method that we need to define for OmittedArgument to make it work correctly so it gets collected and compared to the doc spec validation

[sc68cal]

I think it might be repr() that we need to define

[robduffy2010]

The sanity checking is quite problematic. Ansible also doesn't allow setting the type to custom class.

I've done some testing and it seems we're left with the following options, none of which are ideal, unless you have any other ideas.

1. Use "__OMITTED__" string as the default for each value and then check for that to determine if an argument value was omitted. This means we'll have to change all but string values to type raw. I'll need to test but we could probably have a var_type field in the dict for each param and then have a custom check for that, if we still wanted type checking.

2. Come up with standardised "omitted" default sentinels values for each "type" and then check for those. For example, str is "__OMITTED__", int is 999, dict is {} and so forth.

Thoughts?