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

Allow unracking devices by setting position = 0 #1103

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from

Conversation

eliezerlp
Copy link

Related Issue

No related issue.

New Behavior

Allow setting a device as not racked by setting position: 0 in the data structure.

Contrast to Current Behavior

Currently it is not possible to set a device as Not racked via the netbox.netbox.netbox_device module.

Netbox ordinarily rejects an API call with position=0 since this is outside of the allowable range of values.
The way to ordinarily set a device as unracked is to send null for the particular field.
Since the module strips default fields that have null values (even if explicitly included in the data) it seems otherwise impossible to unrack a device.
Allowing a value that is otherwise rejected by the API seems to be a natural fit.

Discussion: Benefits and Drawbacks

  • Why do you think this project and the community will benefit from your
    proposed change?
    Allows using module in a wider array of use cases.
    My issue was that a device was moved to a particular rack, but the exact RU was not known. When updating the device with the new rack, I couldn't leave the device with the position populated as (1) it was incorrect and (2) it was already populated with another device.

  • What are the drawbacks of this change?
    Not directly mirroring the API, but since null is problematic it seems OK.

  • Is it backwards-compatible?
    It seems backwards compatible since the value 0 is otherwise rejected by the Netbox API since position must be >=1.0.

  • Anything else that you think is relevant to the discussion of this PR.
    My attempts to get the module to pass null were originally foiled by the _remove_arg_spec_default() function. Originally having added a special case to that function, I decided that simply passing a non-null value (which ultimately results in null being sent) was best.

It is worth noting that the position field is one of many default fields that a user might need to set to null. It might we worth considering a more generic approach (perhaps in the future) that addresses any field. For example, one might want to disassociate a device from particular rack. In order to do this, similarly, sending out null is required, but ordinarily stripped even if explicitly included in the data with no value set.

Here is how a device without an assigned position looks:
image

Here is what a changelog entry looks like that actually unracks a device:
image

image

Changes to the Documentation

Not sure how to appropriately document this. I didn't find a spot where it would naturally fit but I do think that documentation is very important since the special meaning of position: 0 is not otherwise known. Here is an example blurb:


Setting Devices as "Not Racked"

A device can be designated as "Not Racked" using the position field in the netbox_device module. To do this, set the position value to 0.

For example:

---
- name: Set Device as Not Racked
  hosts: localhost
  tasks:
    - name: Set a device as "Not Racked"
      netbox.netbox.netbox_device:
        data:
          name: MyDevice
          rack: MyRack
          position: 0
        state: present
        server_url: http://netbox.example.com
        token: YourToken

Proposed Release Note Entry

feat: Set devices to Not Racked by setting position within data to 0 in netbox_device module

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.

Copy link
Contributor

@sc68cal sc68cal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs a unit test, but also frankly I'm not crazy about 0 having a special significance that is specific to this module.

I know there's an issue around somewhere in the tracker discussing how to handle unsetting fields, I'll have to find it

@sc68cal
Copy link
Contributor

sc68cal commented Oct 24, 2023

#330

@eliezerlp
Copy link
Author

Thanks for the link to #330; I hadn't come across it.

If we'd consider merging this approach specific to netbox_device position, I can look into adding unit tests.

@eliezerlp
Copy link
Author

Regarding this:

This needs a unit test, but also frankly I'm not crazy about 0 having a special significance that is specific to this module.

I understand the hesitation regarding 0, but it would be a sensible workaround until #330 solves this more generally.

@sc68cal, please let me know if this would be considered for merging. If so I'll proceed with adding tests. Thanks!

@rodvand
Copy link
Contributor

rodvand commented Feb 14, 2024

I'm happy to merge this and see how users react to having an unracking option. But we need to document this properly. So @eliezerlp , could you add an example to the docs, and point it out in the option documentation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants