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 more Procera interfaces #7422

Merged
merged 13 commits into from Oct 19, 2017

Conversation

Projects
None yet
4 participants
@pheinrichs
Contributor

pheinrichs commented Oct 3, 2017

I've added more ports related to the procera mibs. Please review and let me know if you think this could be done any better or if you have any suggestions.

Thanks :)

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


This change is Reviewable

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 3, 2017

Member

The approach seems reasonable to me.

Member

murrant commented Oct 3, 2017

The approach seems reasonable to me.

pheinrichs added some commits Oct 3, 2017

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 5, 2017

Member

@pheinrichs You might want to merge or rebase this against master to pull in the snmpwalk_group() changes.

Member

murrant commented Oct 5, 2017

@pheinrichs You might want to merge or rebase this against master to pull in the snmpwalk_group() changes.

@pheinrichs

This comment has been minimized.

Show comment
Hide comment
@pheinrichs

pheinrichs Oct 5, 2017

Contributor

@murrant @laf I've added IfSpeed / duplex and also changed the depth to 1 in the snmpgroup_walk. I also revisited the PACKETLOGIC-CHANNEL-MIB to make sure I didn't miss any extra fields. Let me know what you think! :)

Contributor

pheinrichs commented Oct 5, 2017

@murrant @laf I've added IfSpeed / duplex and also changed the depth to 1 in the snmpgroup_walk. I also revisited the PACKETLOGIC-CHANNEL-MIB to make sure I didn't miss any extra fields. Let me know what you think! :)

@laf

laf approved these changes Oct 5, 2017

LGTM

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 5, 2017

Member

@pheinrichs Just a few minor style comments above.

I'm testing this on our PL7810.

The ports from ifMib are mostly useless and duplicates, do you think we should remove them? They do contain valid MAC info though...

Member

murrant commented Oct 5, 2017

@pheinrichs Just a few minor style comments above.

I'm testing this on our PL7810.

The ports from ifMib are mostly useless and duplicates, do you think we should remove them? They do contain valid MAC info though...

@pheinrichs

This comment has been minimized.

Show comment
Hide comment
@pheinrichs

pheinrichs Oct 6, 2017

Contributor

@murrant Them containing macs is what makes me hesitant. I'd vote for leave them?

Contributor

pheinrichs commented Oct 6, 2017

@murrant Them containing macs is what makes me hesitant. I'd vote for leave them?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 6, 2017

Member

I'm saying you should merge them together.

Member

murrant commented Oct 6, 2017

I'm saying you should merge them together.

@pheinrichs

This comment has been minimized.

Show comment
Hide comment
@pheinrichs

pheinrichs Oct 16, 2017

Contributor

@murrant sorry for the delay. I don't think i understand what you would like done. Is there an example i could look off of.

Contributor

pheinrichs commented Oct 16, 2017

@murrant sorry for the delay. I don't think i understand what you would like done. Is there an example i could look off of.

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 18, 2017

Member

I think the airfiber devices do it.

Instead of presenting 16 ports to the user (when there is physically 8), present 8 ports to the user, taking the correct parts of data from if-mib and the procera mibs and combining them.

Basically what you are doing here, but also pull in the if-mib data.

Member

murrant commented Oct 18, 2017

I think the airfiber devices do it.

Instead of presenting 16 ports to the user (when there is physically 8), present 8 ports to the user, taking the correct parts of data from if-mib and the procera mibs and combining them.

Basically what you are doing here, but also pull in the if-mib data.

@pheinrichs

This comment has been minimized.

Show comment
Hide comment
@pheinrichs

pheinrichs Oct 18, 2017

Contributor

I've been comparing the two, and they don't really match up. the IF-MIB generates about 10 ports, but there are only about 3 virtual ports(3 internal/ 3 external) that get discovered from my PR. Is this still the desired way to go?

Contributor

pheinrichs commented Oct 18, 2017

I've been comparing the two, and they don't really match up. the IF-MIB generates about 10 ports, but there are only about 3 virtual ports(3 internal/ 3 external) that get discovered from my PR. Is this still the desired way to go?

@murrant

This comment has been minimized.

Show comment
Hide comment
@murrant

murrant Oct 19, 2017

Member

I looked at it and you're right it doesn't line up.

I've thought about this and think the best solution is to add this to the yaml file:

bad_if_regexp: '/^pl[0-9]+$/'
Member

murrant commented Oct 19, 2017

I looked at it and you're right it doesn't line up.

I've thought about this and think the best solution is to add this to the yaml file:

bad_if_regexp: '/^pl[0-9]+$/'
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Oct 19, 2017

The inspection completed: No new issues

scrutinizer-notifier commented Oct 19, 2017

The inspection completed: No new issues

@murrant murrant merged commit f038bcb into librenms:master Oct 19, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@pheinrichs pheinrichs deleted the pheinrichs:add-procera-interfaces branch May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment