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 Tagged VLAN, work at least with HPE Switch #708

Merged
merged 6 commits into from
Oct 15, 2019

Conversation

PR-gh
Copy link
Contributor

@PR-gh PR-gh commented Aug 6, 2019

Take in charge Vlan for switch HPE (and other switchs) with SNMP :
dot1qVlanStaticName, dot1qVlanStaticRowStatus, dot1qVlanCurrentEgressPorts and dot1qVlanCurrentUntaggedPorts )
Add Information if the Vlan is Tagged or not.
fusioninventory/fusioninventory.github.io#66
fusioninventory/fusioninventory-for-glpi#2852

Copy link
Contributor

@g-bougard g-bougard left a comment

Choose a reason for hiding this comment

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

I see mostly cosmetic issues here. By the way, I'm against introducing a new dependency on Math::BigInt because of just this feature. Please try to do without it.

lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
lib/FusionInventory/Agent/Tools/Hardware.pm Outdated Show resolved Hide resolved
@PR-gh PR-gh requested a review from g-bougard September 30, 2019 09:38
Copy link
Contributor

@g-bougard g-bougard left a comment

Choose a reason for hiding this comment

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

I'm okay with last commits, but there's just a last minor change needed ;-)

lib/FusionInventory/Agent/Tools/SNMP.pm Outdated Show resolved Hide resolved
@PR-gh PR-gh requested a review from g-bougard October 3, 2019 09:19
@g-bougard g-bougard merged commit 0f09745 into fusioninventory:develop Oct 15, 2019
@g-bougard
Copy link
Contributor

Hi @PR-gh
I found finally some problems with this PR. Sometime your code fails to handle dot1qVlanStaticName, dot1qVlanStaticRowStatus, dot1qVlanCurrentEgressPorts and dot1qVlanCurrentUntaggedPorts in the right way. Then it simply removes VLAN information in that case. The problem is you made some wrong assumption about the suffix to use for dot1qVlanCurrentEgressPorts and dot1qVlanCurrentUntaggedPorts. The assumption works on HPE. You also make the assumption the port id is an ordered port number and in fact this is not always true with some other manufacturers.
I can fall back to use .1.0.8802.1.1.2.1 based method when no result is found by your method, but I still see few problems.
I'll push commit soon and I hope you'll be able to test them and validate they don't break anything in your case.

@abmteam
Copy link

abmteam commented Nov 21, 2019

Function _getVlans (in Hardware.pm) doesn't bring all results
#366

g-bougard added a commit that referenced this pull request Nov 21, 2019
Also fix a wrong test.
This is a complement to #708
@g-bougard
Copy link
Contributor

@PR-gh can't validate that b9c5c4b and dad2096 commits do not have any wrong side effect for you ?
@abmteam I checked your issue again. I think This PR plus the last 2 commits may fix your issue. Did you try it ?

@PR-gh
Copy link
Contributor Author

PR-gh commented Nov 22, 2019

Hi @g-bougard,
sorry to listen side effect of my PR for other switchs.

I try on many HPE switchs with dad2096 and no side effect, I have make a diff of inventory xml before/after, only difference it's ordered VLAN in VLANS.

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.

3 participants