-
Notifications
You must be signed in to change notification settings - Fork 39
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
Avoid exception on adding ip device with wrong ip/hostname #2713
base: master
Are you sure you want to change the base?
Avoid exception on adding ip device with wrong ip/hostname #2713
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this solves the problem slightly at the wrong level. This will also conflict heavily with the ongoing changes in #2711 (which has shown me that the connectivity tests on the backend are named and structured in stupid ways).
Anyway, I have a working JS suggestion for checking the backend response and distinguishing between internal server errors and bad requests - and I think a request to check the connectivity of an invalid IP address should return a 400 Bad Request.
However, the functionality currently supports both IP addresses and host names as input. If you enter a hostname that resolves to multiple IP addresses in the netbox form, the JS UI will force you to select one of the IP addresses before allowing a connectivity test. If it only resolves to a single address, the hostname is used as input, and that hostname is fed directly to the underlying SNMP library, resulting in a sort of nondescript exception being raised.
A more proper solution should probably figure out the actual IP address at the Django view level, and return a 400 Bad Request with a JSON error payload if the IP addr is invalid. Actual unhandled errors from the SNMP layer should still cause a 500 internal error, but the front end should treat 500 and 400 differently.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2713 +/- ##
==========================================
- Coverage 55.19% 55.19% -0.01%
==========================================
Files 561 561
Lines 40917 40920 +3
==========================================
+ Hits 22584 22585 +1
- Misses 18333 18335 +2 ☔ View full report in Codecov by Sentry. |
Closes #2696.
Doesn't really show a nice error message, only catches the exception.
If I should show a clearer error message I would need some pointers on how to do that, since I don't see a way to send the error message up