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 write-enabled SNMP profiles to be used for reading when no read-only profiles are available #2751

Merged

Conversation

lunkwill42
Copy link
Member

@lunkwill42 lunkwill42 commented Nov 17, 2023

Fixes #2735

Depends on #2754

@lunkwill42 lunkwill42 self-assigned this Nov 17, 2023
@lunkwill42 lunkwill42 marked this pull request as draft November 17, 2023 13:20
@lunkwill42
Copy link
Member Author

I have yet to properly verify that this works in real life, so keeping in Draft until then...

Copy link

github-actions bot commented Nov 17, 2023

Test results

     12 files       12 suites   12m 15s ⏱️
3 282 tests 3 282 ✔️ 0 💤 0
9 321 runs  9 321 ✔️ 0 💤 0

Results for commit 3ba3a24.

♻️ This comment has been updated with latest results.

@lunkwill42 lunkwill42 force-pushed the feature/fix-preferred-snmp-profile-logic branch 4 times, most recently from cab6497 to a8a3e4b Compare November 20, 2023 11:26
@lunkwill42
Copy link
Member Author

Tested and found to work, but in order to avoid needing to fix issues in the deprecated Netbox properties, this now depends on #2754 to merged first.

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (3b4ec87) 55.62% compared to head (3ba3a24) 55.62%.

Files Patch % Lines
python/nav/models/manage.py 80.00% 2 Missing ⚠️
python/nav/portadmin/snmp/base.py 66.66% 1 Missing ⚠️
python/nav/web/portadmin/views.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2751   +/-   ##
=======================================
  Coverage   55.62%   55.62%           
=======================================
  Files         567      567           
  Lines       41220    41224    +4     
=======================================
+ Hits        22928    22932    +4     
  Misses      18292    18292           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

python/nav/models/manage.py Show resolved Hide resolved
This changes the logic behind preferred SNMP profile selection, a need
that has become apparent while working with SNMPv3 profiles.

A read-write profile should be acceptable for read operations if no
read-only profile exists.  However, if write access is not required,
but a write profile is available in addition to a read profile, the read
profile should be preferred.

This also updates all callers to the new signature, based on their
actual needs.
ManagementProfile.snmp_version now avoids crashing in the case of
faulty SNMP profiles.  Since it was used to sort multiple profiles
in order of SNMP version while fetching preferred profiles, the
presence of any faulty profile on a Netbox would make
`get_preferred_snmp_profile()` unable to fetch any valid profile without
crashing.

This was discovered by our test suite.
@lunkwill42 lunkwill42 force-pushed the feature/fix-preferred-snmp-profile-logic branch from a8a3e4b to 3ba3a24 Compare November 21, 2023 10:11
@lunkwill42 lunkwill42 marked this pull request as ready for review November 21, 2023 10:13
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@lunkwill42 lunkwill42 merged commit 262b5cf into Uninett:master Nov 21, 2023
13 checks passed
@lunkwill42 lunkwill42 deleted the feature/fix-preferred-snmp-profile-logic branch November 21, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants