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

netbox_interface.py missing from collection #124

Closed
gundalow opened this issue Mar 4, 2020 · 12 comments · Fixed by #127
Closed

netbox_interface.py missing from collection #124

gundalow opened this issue Mar 4, 2020 · 12 comments · Fixed by #127
Labels
documentation Improvements or additions to documentation PR Submitted

Comments

@gundalow
Copy link
Contributor

gundalow commented Mar 4, 2020

ISSUE TYPE
  • Bug Report
SUMMARY

Hi,
netbox_interface.py was in ansible/ansible:devel though isn't included in this collection.

Should it be?

STEPS TO REPRODUCE
EXPECTED RESULTS
ACTUAL RESULTS

@FragmentedPacket
Copy link
Contributor

Yes. It was actually converted to netbox_device_interface to differentiate from the two endpoints that use interfaces: devices and virtual machines

There is also a netbox_vm_interface to handle the vm interfaces

@gundalow
Copy link
Contributor Author

gundalow commented Mar 4, 2020

Thanks for the info (and quick response)

netbox_interface was added in Ansible 2.8, so we need to keep that file. At the moment this module is in the community.general collection. I wonder if it would make sense for it to be added to this collection.

@gundalow
Copy link
Contributor Author

gundalow commented Mar 4, 2020

Currently community.general depends on netbox.netbox

@gundalow
Copy link
Contributor Author

gundalow commented Mar 4, 2020

oh, also plugins/inventory/netbox.py is community.general

I'd like to see plugins/inventory/netbox.py and netbox_interface moved into this collection.

@FragmentedPacket
Copy link
Contributor

The inventory plugin has been moved into this collection. I did rename it to nb_inventory due to the fact that calling it looks really ugly and non-intuitive (netbox.netbox.netbox) and then we have the lookup plugin in here as well that would be found at netbox.netbox.netbox as well.

I'd rather not have the netbox_interface in this collection as it has outdated code and all the modules now make use of classes, etc. to prevent as much code duplication as necessary.

@gundalow
Copy link
Contributor Author

gundalow commented Mar 4, 2020

OK,
I've added inventory/netbox to ansible-community/collection_migration#479

My concern about leaving netbox_interface in community.general is that current users may not know about netbox.netbox (ie the docs aren't link)

It's OK for netbox_interface to be in netbox.netbox and marked as deprecated then deleted in the future.

@FragmentedPacket
Copy link
Contributor

I will look into how to signify it is deprecated and get it added to this collection. We can keep this issue open and I'll get it closed once I get it merged into the repo.

Thanks for all the feedback!

@gundalow
Copy link
Contributor Author

gundalow commented Mar 6, 2020

I will look into how to signify it is deprecated and get it added to this collection. We can keep this issue open and I'll get it closed once I get it merged into the repo.

Thanks for all the feedback!

Thank you, given this, I've raised ansible-community/collection_migration#496

I realize there has been some back and forth with all of this while we've been learning. Thak you for your understanding.

@FragmentedPacket
Copy link
Contributor

No problem! I appreciate you reaching out to figure everything out!

@bluikko
Copy link
Contributor

bluikko commented May 5, 2020

I might be the only one with this opinion but I find it very cumbersome to have to always have two plays in Ansible for the VM/device API endpoints. How likely is it that there is a device and a VM with the same name?

@DouglasHeriot
Copy link
Contributor

@bluikko It's not just about the namespace and id space - it's the Netbox API endpoints are separate for devices vs VMs. So even if the same module supported both devices and vms, it would need a parameter to specify if you're working with a device or vm.

@bluikko
Copy link
Contributor

bluikko commented May 6, 2020

@DouglasHeriot I understand that - I'm just suggesting that the collection could make that decision, instead of the user. Anyways it seems I'm the only one who is bothered with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation PR Submitted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants