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

Select generic SNMPHandler for Cisco Small Business switches in PortAdmin #3250

Merged

Conversation

lunkwill42
Copy link
Member

This fixes #3249 in two rounds, after introducing/updating tests for the issue at hand:

  • It first adds a "simple" solution by changing the Management Factory.
  • If then refactors the management handler selection to something that should be more maintainable in the future.

Existing netbox mock fixtures needed to be updated with proper
sysObjectID value mocking as well, since the selection logic for
handlers will become more complex.
Cisco small business switches do not actually work like other Cisco
products SNMP-wise, and we need to revert to the generic handler to
make them work with PortAdmin
This pushes the logic of PortAdmin management handler selection to the
handlers themselves, making it easier to implement more complex
selection logic if needed.  It seems more correct to let the handler
classes decide for themselves whether a given Netbox is compatible.

A `can_handle()` class method is added to `ManagementHandler`, whose
default implementation is based on the existing "selection by `VENDOR`
id" scheme.  The extra logic for Cisco-but-not-quite-Cisco can therefore
be pushed into a specific `can_handle()` implementation for the
Cisco handler.
Copy link

github-actions bot commented Nov 29, 2024

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ PYTHON black 6 0 0.72s
✅ PYTHON ruff 6 0 0.01s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@lunkwill42 lunkwill42 self-assigned this Nov 29, 2024
@lunkwill42 lunkwill42 marked this pull request as ready for review November 29, 2024 10:42
@lunkwill42 lunkwill42 requested a review from a team November 29, 2024 10:42
Copy link

github-actions bot commented Nov 29, 2024

Test results

    9 files      9 suites   8m 13s ⏱️
2 148 tests 2 148 ✅ 0 💤 0 ❌
4 035 runs  4 035 ✅ 0 💤 0 ❌

Results for commit b6c327d.

♻️ This comment has been updated with latest results.

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.34%. Comparing base (d385dca) to head (b6c327d).
Report is 14 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3250   +/-   ##
=======================================
  Coverage   60.33%   60.34%           
=======================================
  Files         606      606           
  Lines       43700    43710   +10     
  Branches       48       48           
=======================================
+ Hits        26366    26376   +10     
  Misses      17322    17322           
  Partials       12       12           

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

@johannaengland johannaengland self-requested a review December 3, 2024 14:09
Make the fallback algorithm more explicit, as per code review
comments.
@lunkwill42 lunkwill42 requested a review from stveit December 4, 2024 09:19
Copy link

sonarcloud bot commented Dec 4, 2024

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.

I really like the new refactor

@lunkwill42 lunkwill42 merged commit 769d1d2 into master Dec 4, 2024
15 checks passed
@lunkwill42 lunkwill42 deleted the feature/portadmin-select-generic-handler-for-ciscosmb branch December 4, 2024 12:35
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.

Setting access port VLAN on a Cisco Small Business switch from PortAdmin should work
3 participants