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 Netbox device modification #53631

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
6 participants
@Sweenu
Copy link
Contributor

Sweenu commented Mar 11, 2019

SUMMARY

This PR allows a Netbox device modification on top of the existing creation and deletion actions.

The ability to modify a device introduces risks if there are any mistakes. That is why I also implemented check mode and the diff output.
I also removed some mechanism that used default values silently when the given value was not found (see diff of line 145 in netbox_utils.py) in order to make sure a mistake in using the module would raise an error instead of silently setting arbitrary values.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

netbox_device.py

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch 2 times, most recently from 70da6f0 to 4ac6156 Mar 11, 2019

@ansibot

This comment has been minimized.

@Anthony25

This comment has been minimized.

Copy link
Contributor

Anthony25 commented Mar 11, 2019

LGTM

1 similar comment
@sieben

This comment has been minimized.

Copy link
Contributor

sieben commented Mar 11, 2019

LGTM

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

FragmentedPacket commented Mar 11, 2019

Could we get the diff/update pushed into Netbox utils? I would assume these are something we would want to be added to every other module as well.

@ansibot ansibot removed the needs_triage label Mar 11, 2019

@Sweenu

This comment has been minimized.

Copy link
Contributor Author

Sweenu commented Mar 11, 2019

Good idea! I'm working on it, will push something tonight.

@Sweenu

This comment has been minimized.

Copy link
Contributor Author

Sweenu commented Mar 11, 2019

This last commit is what I came up with, tell me what you think. I might add some tests tomorrow, that'd be nice. If not, I'll probably do it in another PR.

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

FragmentedPacket commented Mar 11, 2019

That would be great! We have three modules needing approval but are requiring tests and that is something new to me. If you could provide some for this module that I can use for the other, that'd be great!

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch 2 times, most recently from bd6e789 to e713a62 Mar 11, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 12, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:0: syntax-error invalid syntax (<unknown>, line 154)

The test ansible-test sanity --test compile --python 2.6 [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:32: SyntaxError: nb_obj.update(data):

The test ansible-test sanity --test compile --python 2.7 [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:32: SyntaxError: nb_obj.update(data):

The test ansible-test sanity --test compile --python 3.5 [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:32: SyntaxError: nb_obj.update(data):

The test ansible-test sanity --test compile --python 3.6 [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:33: SyntaxError: nb_obj.update(data):

The test ansible-test sanity --test compile --python 3.7 [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:33: SyntaxError: nb_obj.update(data):

The test ansible-test sanity --test compile --python 3.8 [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:33: SyntaxError: nb_obj.update(data):

The test ansible-test sanity --test import --python 2.6 [explain] failed with 3 errors:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:32: SyntaxError: invalid syntax
lib/ansible/modules/net_tools/netbox/netbox_device.py:176:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)
lib/ansible/modules/net_tools/netbox/netbox_ip_address.py:226:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)

The test ansible-test sanity --test import --python 2.7 [explain] failed with 3 errors:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:32: SyntaxError: invalid syntax
lib/ansible/modules/net_tools/netbox/netbox_device.py:176:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)
lib/ansible/modules/net_tools/netbox/netbox_ip_address.py:226:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)

The test ansible-test sanity --test import --python 3.5 [explain] failed with 3 errors:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:32: SyntaxError: invalid syntax
lib/ansible/modules/net_tools/netbox/netbox_device.py:176:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)
lib/ansible/modules/net_tools/netbox/netbox_ip_address.py:226:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)

The test ansible-test sanity --test import --python 3.6 [explain] failed with 3 errors:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:33: SyntaxError: invalid syntax
lib/ansible/modules/net_tools/netbox/netbox_device.py:176:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)
lib/ansible/modules/net_tools/netbox/netbox_ip_address.py:226:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)

The test ansible-test sanity --test import --python 3.7 [explain] failed with 3 errors:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:33: SyntaxError: invalid syntax
lib/ansible/modules/net_tools/netbox/netbox_device.py:176:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)
lib/ansible/modules/net_tools/netbox/netbox_ip_address.py:226:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)

The test ansible-test sanity --test import --python 3.8 [explain] failed with 3 errors:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:154:33: SyntaxError: invalid syntax
lib/ansible/modules/net_tools/netbox/netbox_device.py:176:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)
lib/ansible/modules/net_tools/netbox/netbox_ip_address.py:226:0: SyntaxError: invalid syntax (netbox_utils.py, line 154)

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/module_utils/net_tools/netbox/netbox_utils.py:155:13: E112 expected an indented block

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

lib/ansible/modules/net_tools/netbox/netbox_device.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'invalid syntax (netbox_utils.py, line 154)'
lib/ansible/modules/net_tools/netbox/netbox_ip_address.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'invalid syntax (netbox_utils.py, line 154)'

click here for bot help

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch from 4a820f3 to 1eb00e0 Mar 12, 2019

@ansibot ansibot removed the ci_verified label Mar 12, 2019

@Sweenu

This comment has been minimized.

Copy link
Contributor Author

Sweenu commented Mar 12, 2019

Yeah no problem, I'll add unit tests. I should have done that in the first place anyway.

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch 2 times, most recently from 99f4ca7 to 80bada8 Mar 13, 2019

@Sweenu

This comment has been minimized.

Copy link
Contributor Author

Sweenu commented Mar 13, 2019

I made a few tests. They could be improved and there is still many other tests to write but I don't really think this PR should get much bigger though. It probably is better for me to open a new PR focused on adding some more tests :)

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch 2 times, most recently from 2070c7e to e45f2bf Mar 13, 2019

@Sweenu

This comment has been minimized.

Copy link
Contributor Author

Sweenu commented Mar 14, 2019

ready_for_review

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

FragmentedPacket commented Mar 14, 2019

I will review everything tonight! Thanks for writing the tests!

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

FragmentedPacket commented Mar 15, 2019

@Sweenu Alright after looking at this. We should get rid of the changes to the netbox_ip_address.py and we'll open another PR for that.

I would also remove the def delete_netbox_object(nb_obj, check_mode): and def create_netbox_object(nb_endpoint, data, check_mode): from netbox_utils and keep that stuff within the module as it can be different depending on what the object is and just add logic for the update_netbox_object.

As for the update_netbox_object, I would keep that in the netbox_utils.py, but I'm not sure if I like the way the update is happening. Would it be best to create another function to replace the _build_diff with it actually returning a dictionary of the diff (changes the user wants to be applied) between the proposed data that the user passed into the model and what the object is currently set as. I'd then return that and just update the object with those updates so it is only updating what is different. Does that make sense or does it even matter?

@Sweenu

This comment has been minimized.

Copy link
Contributor Author

Sweenu commented Mar 15, 2019

@FragmentedPacket Thanks for your review!
I'll revert my changes on netbox_ip_address.py then, it's probably better indeed to split up this PR.

I would also remove the def delete_netbox_object(nb_obj, check_mode): and def create_netbox_object(nb_endpoint, data, check_mode): from netbox_utils and keep that stuff within the module as it can be different depending on what the object is and just add logic for the update_netbox_object.

They are simply wrapping the create() and delete() methods with some logic to add check_mode and return a diff. I don't see for what Netbox object we could not use those function.

IMO, It's only a first step towards a bigger and better refactoring consisting of a NetboxModule class that would sit in netbox_utils.py and deduplicate even more similar code found in every current and future netbox modules that would only inherit of this class and make their specific changes. Many other modules are done that way.

For update_netbox_object(), yeah it's not the best but I think it's a good enough first step towards what I'm talking about above. What do you think?

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

FragmentedPacket commented Mar 17, 2019

Sorry about that! I totally misread the code for the create/delete. I plan to test this tomorrow while traveling and then hopefully we can get it merged ASAP.

May I ask for your help writing tests for the three new modules that have PRs open and we can convert those over to the new shared functions as well?

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

FragmentedPacket commented Mar 17, 2019

netbox_ip_address.py
L442: fix . to , before/after data

netbox_utils.py
L122: Add try/except block to catch AttributeError and do not .serialize() creation

Once those are fixed then I'd say it's good to go

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch 2 times, most recently from 6b707f9 to f21822e Mar 18, 2019

@Sweenu

This comment has been minimized.

Copy link
Contributor Author

Sweenu commented Mar 18, 2019

Done!
No problem, I can help with the tests :)

@FragmentedPacket

This comment has been minimized.

Copy link
Contributor

FragmentedPacket commented Mar 18, 2019

LGTM

Show resolved Hide resolved test/units/module_utils/net_tools/test_netbox.py Outdated
Show resolved Hide resolved test/units/module_utils/net_tools/test_netbox.py Outdated
Show resolved Hide resolved test/units/module_utils/net_tools/test_netbox.py Outdated
@@ -198,13 +205,18 @@ def main():
validate_certs=dict(type="bool", default=True)
)

global module

This comment has been minimized.

@NilashishC

NilashishC Mar 25, 2019

Contributor

Please refrain from using global.

This comment has been minimized.

@Sweenu

Sweenu Mar 25, 2019

Author Contributor

It's not ideal indeed and when refactoring Netbox modules into a NetboxModule class, we'll get rid of it. But for now I think it's good enough. That is how it's done in all of those modules:

lib/ansible/modules/identity/onepassword_facts.py
lib/ansible/modules/web_infrastructure/nginx_status_facts.py
lib/ansible/modules/web_infrastructure/jira.py
lib/ansible/modules/files/file.py
lib/ansible/modules/system/openwrt_init.py
lib/ansible/modules/system/parted.py
lib/ansible/modules/net_tools/ipify_facts.py
lib/ansible/modules/monitoring/spectrum_device.py
lib/ansible/modules/packaging/language/pip_package_info.py
lib/ansible/modules/packaging/os/package_facts.py
lib/ansible/modules/storage/glusterfs/gluster_volume.py
lib/ansible/modules/storage/glusterfs/gluster_heal_facts.py
lib/ansible/modules/network/cloudengine/ce_netstream_global.py
lib/ansible/modules/network/cloudengine/ce_vxlan_global.py
lib/ansible/modules/network/cloudengine/ce_info_center_global.py
lib/ansible/modules/network/cloudengine/ce_evpn_global.py
lib/ansible/modules/network/cloudengine/ce_bfd_global.py
lib/ansible/modules/cloud/misc/terraform.py
lib/ansible/modules/cloud/misc/rhevm.py
lib/ansible/modules/cloud/memset/memset_dns_reload.py
lib/ansible/modules/cloud/memset/memset_zone_domain.py
lib/ansible/modules/cloud/memset/memset_server_facts.py
lib/ansible/modules/cloud/memset/memset_zone_record.py
lib/ansible/modules/cloud/memset/memset_zone.py
lib/ansible/modules/cloud/memset/memset_memstore_facts.py
lib/ansible/modules/cloud/cloudstack/cs_facts.py
lib/ansible/modules/cloud/amazon/ec2_asg.py
lib/ansible/modules/cloud/amazon/ec2_instance.py

What do you think? Is it okay for now?

@ansibot ansibot added needs_revision and removed core_review labels Mar 25, 2019

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch from f21822e to 5ffb35f Mar 25, 2019

Sweenu added some commits Mar 7, 2019

netbox_device: Allow device modification
* Add ability to update and existing device
* Allow check_mode
* Fail when device name is missing
* Fail when cannot resolve ID instead of taking ID 1 by default
netbox: Some refactoring
* Add diff output and check_mode to netbox_ip_address
* Deduplicate redundant code into netbox_utils

@Sweenu Sweenu force-pushed the Sweenu:allow-device-modification branch from 5ffb35f to 5d3f0ec Mar 25, 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.