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

Add netbox_ipam_role resource #86

Merged
merged 2 commits into from
Oct 27, 2021

Conversation

Sanverik
Copy link
Contributor

No description provided.

@Sanverik Sanverik marked this pull request as ready for review October 26, 2021 09:55
@Sanverik
Copy link
Contributor Author

@fbreckle please, review

@fbreckle
Copy link
Collaborator

I wonder if role is a good name for the resource. Clicking through the GUI, I see "Rack Roles", "Device Roles" and finally "Prefix/IPAM Roles" (which this MR is about). It might be confusing for the users.

It definitely threw me off, when I read through the MR, but then again, I do not do much IPAM myself. Is it obvious that "role" refers to the roles in the IPAM tab?

Any opionion on that?

Note: The API just calls the resource "Role" as well

@Sanverik
Copy link
Contributor Author

In my point of view, all resources should have an additional prefix relaying on this folder structure:
image
So the name would be looking as netbox_<client>_<actual_resource_name> then you won’t have such misunderstandings or issues with naming.
BUT there are a lot of resources that already exist and do not follow this approach in your provider. I called it role because of:

  • it’s only such resource with role suffix for now
  • as you mentioned, the API also calls the resource as role

@Sanverik
Copy link
Contributor Author

Sanverik commented Oct 26, 2021

So netbox_role of netbox_ipam_role or <your_variant>? For me, it doesn't matter. I could do changes, just choose the right option for you

@fbreckle

@fbreckle
Copy link
Collaborator

fbreckle commented Oct 26, 2021

I'd go with netbox_ipam_role.

For all previous items, IMHO it is unnecesarry to use a prefix, i.e. I find netbox_virtualization_virtual_machine somewhat unhandy. Role is the first one that feels unambiguous (edit: i mean ambiguous, obviously), so we should clarify.

Since these roles are for both prefixes and VLANs, netbox_prefix_role or netbox_vlan_role cannot be used.

@Sanverik Sanverik changed the title Add netbox_role resource Add netbox_ipam_role resource Oct 26, 2021
@Sanverik
Copy link
Contributor Author

@fbreckle review it again, please

@fbreckle fbreckle merged commit 458c500 into e-breuninger:master Oct 27, 2021
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.

2 participants