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.

Sign up for GitHub

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

Jump to bottom

Allow Netbox device modification #53631

Merged
merged 4 commits into from
Apr 7, 2019
Merged

Allow Netbox device modification #53631

merged 4 commits into from
Apr 7, 2019

Conversation

sweenu
Copy link
Contributor

@sweenu sweenu commented Mar 11, 2019 •
edited

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 allow-device-modification branch 2 times, most recently from 70da6f0 to 4ac6156 Compare March 11, 2019 14:52
@ansibot
Copy link
Contributor

ansibot commented Mar 11, 2019

cc @FragmentedPacket @amb1s1 @FragmentedPacket
click here for bot help

@ansibot ansibot added affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team. community_review In order to be merged, this PR must follow the community review workflow. and removed core_review In order to be merged, this PR must follow the core review workflow. support:core This issue/PR relates to code supported by the Ansible Engineering Team. labels Mar 11, 2019
@aruhier
Copy link
Contributor

aruhier commented Mar 11, 2019

LGTM

1 similar comment
@remyleone
Copy link
Contributor

LGTM

@FragmentedPacket
Copy link
Contributor

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 Needs a first human triage before being processed. label Mar 11, 2019
@sweenu
Copy link
Contributor Author

sweenu commented Mar 11, 2019

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

@sweenu
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
Copy link
Contributor

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 allow-device-modification branch 2 times, most recently from bd6e789 to e713a62 Compare March 11, 2019 21:55
@ansibot

This comment has been minimized.

Sign in to view
@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Mar 12, 2019
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Mar 12, 2019
@sweenu
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.

@ansibot ansibot added the core_review In order to be merged, this PR must follow the core review workflow. label Mar 26, 2019
@jqueuniet jqueuniet mentioned this pull request Mar 27, 2019
Closed
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Apr 3, 2019
@NilashishC
Copy link
Contributor

rebuild_merge

@ansibot ansibot added needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed core_review In order to be merged, this PR must follow the core review workflow. labels Apr 3, 2019
@NilashishC
Copy link
Contributor

@sweenu this needs rebase.

@FragmentedPacket
Copy link
Contributor

@sweenu Sorry about the need to rebase. I went ahead and implemented your changes in some new modules which caused the conflicts. I think the main differences are the raising ValueError on L194-L205. In your rebase, do you mind leaving the elif k in NO_DEFAULTID: pass within there as well?

@FragmentedPacket
Copy link
Contributor

@NilashishC If 2.8 Alpha is released today - can this still be implemented into 2.8?

@sweenu sweenu force-pushed the allow-device-modification branch from 5d3f0ec to e0f1e91 Compare April 4, 2019 23:05
Sweenu added 2 commits April 5, 2019 01:07
netbox_device: Allow device modification
d6ecb1e 
* 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_device: Add diff output
9853e3d
@sweenu sweenu force-pushed the allow-device-modification branch from e0f1e91 to 1110238 Compare April 4, 2019 23:08
@sweenu
Copy link
Contributor Author

sweenu commented Apr 4, 2019

No problem, conflicts resolved :)

@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Apr 4, 2019
@FragmentedPacket
Copy link
Contributor

Thanks for those! I'm not sure if my phone isn't tendering correctly, but are there functions duplicated?

Sweenu added 2 commits April 5, 2019 11:49
netbox: Some refactoring
12b7018 
* Add diff output and check_mode to netbox_ip_address
* Deduplicate redundant code into netbox_utils
netbox_utils: A few unit tests
e744c5c
@sweenu sweenu force-pushed the allow-device-modification branch from 1110238 to e744c5c Compare April 5, 2019 09:51
@sweenu
Copy link
Contributor Author

sweenu commented Apr 5, 2019

Sorry indeed, I messed up.

@FragmentedPacket
Copy link
Contributor

shipit

@FragmentedPacket
Copy link
Contributor

Thanks for all your help with this! I'll probably reach out to you via email to discuss refactoring

@NilashishC NilashishC merged commit c637104 into ansible:devel Apr 7, 2019
@FragmentedPacket FragmentedPacket mentioned this pull request Apr 10, 2019
Merged
@ansible ansible locked and limited conversation to collaborators Jul 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@NilashishC NilashishC NilashishC approved these changes

@FragmentedPacket FragmentedPacket FragmentedPacket approved these changes

Assignees
No one assigned
Labels
affects_2.8 This issue/PR affects Ansible v2.8 core_review In order to be merged, this PR must follow the core review workflow. feature This issue/PR relates to a feature request. module This issue/PR relates to a module. net_tools Net-tools category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@sweenu @ansibot @aruhier @remyleone @FragmentedPacket @NilashishC