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

Fix for RouterOS LLDP discovery #10265

Merged
merged 3 commits into from May 28, 2019

Conversation

Projects
None yet
2 participants
@mjducharme
Copy link
Contributor

commented May 26, 2019

Only the first nine neighbors were correct on RouterOS - missing hexdec() led to the remaining neighbors being linked to the wrong ports. Ex. port 10 in hex was incorrectly linked to port 10 decimal when it should be 16 in decimal

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Fix for RouterOS LLDP discovery
Only the first nine neighbors were correct on RouterOS - missing hexdec() led to the remaining neighbors being linked to the wrong ports. Ex. port 10 in hex was incorrectly linked to port 10 decimal when it should be 16 in decimal
@label-actions

This comment has been minimized.

Copy link

commented May 26, 2019

Please add test data so we can ensure your change is not broken in the future.

Read the docs to find out how: https://docs.librenms.org/Developing/os/Test-Units

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

Hi @mjducharme
Looks good so far. Could you also generate some test data (new variant for your family of device) with a few LLDP neighbors at least ? The more feature enabled you have the better (meaning the testdata will cover a wider part of the RouterOS code).
Thx

mjducharme added some commits May 26, 2019

@mjducharme

This comment has been minimized.

Copy link
Contributor Author

commented May 26, 2019

Test data submitted

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Test data submitted

I don't see any neighbor data in it ? Could you run a test data collection with at least one or 2 neighbors ?

@mjducharme

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

That device has 18 neighbors. You can see them in the .snmprec. I was surprised that they do not show up in the .json file but since I've never done this before I thought that might be normal. Are they supposed to appear in the .json file somehow?

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented May 28, 2019

Most probably because the neighbors are default disabled. So they are not tested. At least the snmprec is here.

@PipoCanaja
Copy link
Contributor

left a comment

LGTM

@PipoCanaja PipoCanaja merged commit 902c056 into librenms:master May 28, 2019

6 checks passed

Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
codeclimate All good!
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.