-
Notifications
You must be signed in to change notification settings - Fork 5
Implement an SNMP back-end selection option in zino.toml
#396
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 an SNMP back-end selection option in zino.toml
#396
Conversation
🦙 MegaLinter status: ✅ SUCCESS
See detailed report in MegaLinter reports |
Test results 3 files 3 suites 1m 15s ⏱️ Results for commit 77c07f6. ♻️ This comment has been updated with latest results. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #396 +/- ##
==========================================
+ Coverage 98.46% 98.59% +0.14%
==========================================
Files 83 83
Lines 9971 10021 +50
==========================================
+ Hits 9817 9880 +63
+ Misses 154 141 -13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zino.toml
zino.toml
9df3750
to
acf514d
Compare
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.
It would be good to document somewhere that it is not enough to change the snmp.backend
setting in the config for a running Zino, but that it needs to be restarted for it to come into effect
Doesn't every change to |
Yes, it does, I mixed up |
a97452c
to
decef88
Compare
acf514d
to
82ed56a
Compare
decef88
to
1ed57cf
Compare
82ed56a
to
0745cbb
Compare
0745cbb
to
5895c86
Compare
Keeping this in draft mode until I'm satisfied with test coverage |
0e9cd93
to
cceb873
Compare
d62026d
to
f066705
Compare
Coverage looks ok now, so taking out of draft |
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.
See questions
cceb873
to
980ac89
Compare
This adds functionality to `zino.snmp` and `zino.trapd` to select between the supported back-ends. This is mostly geared at loading a single default back-end for the running process, but will allow for importing more than one back-end, for example for testing scenarios.
Instead of the Zino main program initializing the netsnmpy backend explicitly, ensure the call goes out to the selected backend instead.
This just ensure the example program can keep running.
A back-end needs to be loaded before much of the Zino code can be tested, but since multiple back-ends can eventually be imported, the test suite can still target explicit back-ends if needed.
20b06d0
to
4eccdf0
Compare
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.
Just a nitpick about test names 😁
Because I changed two lines to make the script work, CodeCov insists I need coverage of those lines - but getuptime.py had no coverage to begin with. Not really very relevant to this particular PR, but it turns out that getuptime.py actually didn't work as intended, which these tests discovered - so overall, this is a good thing :)
As commented in code-review.
4eccdf0
to
77c07f6
Compare
|
Scope and purpose
Part of fix for #383. Dependent on #394 and #395.
This adds an option
snmp.backend
to the configuration filezino.toml
(defaulting to the newnetsnmp-cffi
back-end).Since
netsnmp-cffi
is a new back-end, still in development, this allows users to optionally go back to the original PySNMP-based back-end in case they experience stability issues with the new solution.This pull request
zino.toml
Contributor Checklist
Every pull request should have this checklist filled out, no matter how small it is.
More information about contributing to Zino can be found in the
README.