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

Implement netbox_tenant module #58140

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
4 participants
@amylieb
Copy link

commented Jun 20, 2019

SUMMARY

I've started leveraging @FragmentedPacket's netbox modules and found I wanted a "netbox_tenant" module, so I copied netbox_site and altered it to manipulate the tenant endpoint.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

netbox_tenant - module for getting/creating 'tenant' netbox endpoints.

ADDITIONAL INFORMATION

Adapting netbox_site to create netbox_tenant was straightforward. No changes to netbox_utils were needed.


@amylieb amylieb referenced this pull request Jun 20, 2019

Closed

Add netbox_tenant module #58137

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2019

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/modules/net_tools/netbox/netbox_tenant.py:198:28: bad-whitespace Exactly one space required after comma     _check_and_adapt_data(nb,data)                             ^
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:229:28: bad-whitespace Exactly one space required after comma def _check_and_adapt_data(nb,data):                             ^
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:233:28: bad-whitespace Exactly one space required after comma       group_id = find_ids(nb,{"tenant_group":_slugify(data["group"])})                             ^

The test ansible-test sanity --test pep8 [explain] failed with 7 errors:

lib/ansible/modules/net_tools/netbox/netbox_tenant.py:198:29: E231 missing whitespace after ','
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:217:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:229:29: E231 missing whitespace after ','
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:233:7: E111 indentation is not a multiple of four
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:233:29: E231 missing whitespace after ','
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:233:45: E231 missing whitespace after ':'
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:234:7: E111 indentation is not a multiple of four

The test ansible-test sanity --test validate-modules [explain] failed with 2 errors:

lib/ansible/modules/net_tools/netbox/netbox_tenant.py:0:0: E307 version_added should be '2.9'. Currently '2.8'
lib/ansible/modules/net_tools/netbox/netbox_tenant.py:0:0: E337 Argument 'data' in argument_spec defines type as 'dict' but documentation doesn't define type

click here for bot help

@jillr

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

Thanks for this contribution!
It looks like there's some test failures that will need to be addressed for this module. You can run the command the bot is showing above each of the test failures on your local machine to test if changes you make will pass the tests before pushing any fixes up to github.

We'd also appreciate if you would be willing to write integration tests for the new module. Integration tests generally take the form of ansible playbooks that exercise the functionality of the module, accounting for different options or configurations as necessary. They go in a specific directory structure that allows CI to know about them and execute them properly. Unfortunately it looks like we don't yet have any tests for the other netbox modules to compare to, but we have some general info here:
https://docs.ansible.com/ansible/latest/dev_guide/testing_integration.html#testing-integration
And you might be able to get some useful info from some of the other network tools that have integration tests, like https://github.com/ansible/ansible/tree/devel/test/integration/targets/nios_network or https://github.com/ansible/ansible/tree/devel/test/integration/targets/slurp.

@ansibot ansibot removed the needs_triage label Jun 21, 2019

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@jillr I'm currently working on refactoring the netbox_utils and then increasing the amount of tests and types of tests.

I'd like to do integration testing as that will provide better coverage. Is there anyway to have the tests spin up some docker containers for Netbox to run the tests against?

@amylieb amylieb force-pushed the amylieb:netbox_tenant branch from a18a61b to 4b80286 Jun 21, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

@Anthony25 @FragmentedPacket @amb1s1

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@amylieb

This comment has been minimized.

Copy link
Author

commented Jun 21, 2019

Fixed sanity check issues. Am more than happy to write integration tests, but any meaningful test would require a Netbox instance to test against. Echoing @FragmentedPacket's request to be able to spin up Netbox instance(s) in containers as part of the testing process.

@jillr

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

That's great to hear @FragmentedPacket, we can do docker images but it's a bit more work to get setup so we'd want to do that in a separate PR. You can take a look at the cloudstack (cs_ prefix) test suite as an example. So for this PR no tests would be totally fine if they're in the works.

@ansibot ansibot added the stale_ci label Jun 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.