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

Add SNMPv3 support to Portadmin #2731

Merged
merged 4 commits into from Nov 16, 2023

Conversation

lunkwill42
Copy link
Member

Tested successfully against our lab switches.

Closes #2712

Copy link

github-actions bot commented Nov 13, 2023

Test results

     12 files       12 suites   11m 25s ⏱️
3 256 tests 3 256 ✔️ 0 💤 0
9 243 runs  9 243 ✔️ 0 💤 0

Results for commit 493e595.

♻️ This comment has been updated with latest results.

@lunkwill42 lunkwill42 changed the base branch from snmpv3-synchronous to master November 13, 2023 13:51
Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're removing the use of Snmp, the import at the beginning should also be removed. Otherwise all looks good

@lunkwill42 lunkwill42 closed this Nov 16, 2023
@lunkwill42 lunkwill42 reopened this Nov 16, 2023
Netbox.get_preferred_snmp_manage_profile() should be trusted to return
a proper SNMP profile.  I see no reason to guard against faulty
return values like this.
Lets the utility function set up the SNMP session for SNMPHandler based
management handlers in PortAdmin.
The profile fixture returned a mocked management profile object that
was incomplete.  The new SNMP setup routines inspect the profile
configuration more closely, and would fail.  There is no need to use
a Mock to mock a profile, just create an instance with an actual
configuration dict, but just don't save it anywhere.
@lunkwill42
Copy link
Member Author

Changes to Portadmin and its tests were recently merged to master, and my latest changes were in conflict with these. Had to force-push an updated version to make things mergeable.

Copy link

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (75a6331) 55.46% compared to head (493e595) 55.46%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2731      +/-   ##
==========================================
- Coverage   55.46%   55.46%   -0.01%     
==========================================
  Files         567      567              
  Lines       41175    41172       -3     
==========================================
- Hits        22838    22836       -2     
+ Misses      18337    18336       -1     

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

Copy link

sonarcloud bot commented Nov 16, 2023

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
Copy link
Member Author

If you're removing the use of Snmp, the import at the beginning should also be removed. Otherwise all looks good

Good point. Even though they are marked as unused by my IDE, we don't have linter rules that will trigger this (SonarCloud apparently doesn't, and our pylint setup is defunct). Pushed an extra commit for this cleanup.

@lunkwill42 lunkwill42 merged commit 854efb7 into Uninett:master Nov 16, 2023
13 checks passed
@lunkwill42 lunkwill42 deleted the feature/snmpv3-portadmin branch November 16, 2023 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test and update portadmin management handlers for SNMPv3 session compatibilty
3 participants