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

Inconsistency in API for field role/device #6391

Closed
leahoswald opened this issue May 11, 2021 · 7 comments
Closed

Inconsistency in API for field role/device #6391

leahoswald opened this issue May 11, 2021 · 7 comments
Assignees
Labels
status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user

Comments

@leahoswald
Copy link

NetBox version

v2.9.9

Python version

3.6

Steps to Reproduce

  • Open a VM in the API view: The role field is called role
  • Open a device in the API view: The role field is called device_role

Expected Behavior

I would expect that both fields are called the same (role) because they are linking to the same data.

Observed Behavior

The device field is called device_role and the VM field is called role

@leahoswald leahoswald added the type: bug A confirmed report of unexpected behavior in the application label May 11, 2021
@jeremystretch
Copy link
Member

It doesn't seem like the hassle of introducing a breaking change like this would be worthwhile just to achieve consistency with another model.

@leahoswald
Copy link
Author

leahoswald commented May 12, 2021

Of course it's a breaking change. On the other hand it means extra handling for every code that works with both devices and VMs. For example if you want to generate monitoring config from it. I understand that it is to much for a normal release but maybe if there is a future release with other braking changes it would be an opportunity to fix it. It would be the first braking change on the API (I remember fixing some code because of a change in the state variable.)

@jeremystretch
Copy link
Member

On the other hand it means extra handling for every code that works with both devices and VMs.

These are always going to require separate logic, as they are different models. Renaming the device_role field won't change that.

@jeremystretch jeremystretch added status: under review Further discussion is needed to determine this issue's scope and/or implementation and removed type: bug A confirmed report of unexpected behavior in the application labels May 12, 2021
@jeremystretch
Copy link
Member

Flagging this for milestone assignment to potentially handle in the future. We could potentially implement a getter & setter to maintain backward compatibility across releases, however I'm not sure what potential pitfalls we'd run into regarding Django's ORM's behavior.

@jeremystretch jeremystretch added needs milestone Awaiting prioritization for inclusion with a future NetBox release type: housekeeping Changes to the application which do not directly impact the end user and removed status: under review Further discussion is needed to determine this issue's scope and/or implementation labels May 18, 2021
@abhi1693
Copy link
Member

Would it fine if a field is renamed and this is introduced as a breaking change for v3.5? I can submit a PR if that's accepted as a solution

@ryanmerolle
Copy link
Contributor

This could be a good candidate for 3.6

@abhi1693 abhi1693 self-assigned this Aug 2, 2023
@abhi1693 abhi1693 added status: accepted This issue has been accepted for implementation and removed needs milestone Awaiting prioritization for inclusion with a future NetBox release labels Aug 2, 2023
@abhi1693
Copy link
Member

abhi1693 commented Aug 2, 2023

I'm going to try to make the field backward-compaitible for the API and ORM access. If that's possible, should be able to ship this with v3.6

abhi1693 added a commit that referenced this issue Aug 2, 2023
jeremystretch added a commit that referenced this issue Aug 2, 2023
* replaces device_role with role on device model #6391

* fixes lint issue #6391

* revert the database user

* revert test_runner comment

* changes as per review

* Update references to device_role column in UserConfigs

---------

Co-authored-by: Jeremy Stretch <[email protected]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: accepted This issue has been accepted for implementation type: housekeeping Changes to the application which do not directly impact the end user
Projects
None yet
Development

No branches or pull requests

4 participants