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

New OS: Support for Glass Way EYDFA WDM Optical Amplifier #9125

Merged
merged 29 commits into from Oct 12, 2018

Conversation

Projects
None yet
4 participants
@TheGreatDoc
Copy link
Contributor

TheGreatDoc commented Sep 1, 2018

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

NOTE: Those devices come all with same sysName so $config['allow_duplicate_sysName'] = true; is needed to have more than 1 working.

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor Author

TheGreatDoc commented Sep 1, 2018

Im stucked here. Dont know why Travis complains about sensors, i've not changed them, only did formattings fixes.

Show resolved Hide resolved includes/definitions/gw-eydfa.yaml
- { graph: device_temperature, text: 'Temperatures' }
- { graph: device_dbm, text: 'Optics' }
mib_dir:
- gw-eydfa

This comment has been minimized.

Copy link
@laf

laf Sep 1, 2018

Member

Change the mib dir to glassway

This comment has been minimized.

Copy link
@TheGreatDoc

TheGreatDoc Sep 1, 2018

Author Contributor

I'll do! 👌

* @author TheGreatDoc
*/
$hardware = snmp_get($device, ".1.3.6.1.4.1.17409.1.3.3.2.2.1.4.1", "-OQv");

This comment has been minimized.

Copy link
@laf

laf Sep 1, 2018

Member

Do you not have the OID names to use here? Also needs converting to multi gets.

This comment has been minimized.

Copy link
@TheGreatDoc

TheGreatDoc Sep 1, 2018

Author Contributor

Yes. I dont know why I used numeric.

Show resolved Hide resolved includes/discovery/sensors/gw-eydfa.inc.php

@murrant murrant added the Device 🖥 label Sep 1, 2018

@laf laf added the Blocker 🚫 label Sep 2, 2018

@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 2, 2018

I'll give this a shot to move to yml.

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor Author

TheGreatDoc commented Sep 3, 2018

I can try again with yml, but I'll need some help in how to select proper values for te table that contains different sensor types (Current and Voltage) and in the parse of the limits indexes (yml parse part of the oid as index)

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor Author

TheGreatDoc commented Sep 9, 2018

@laf I moved to yaml. The only I cant get working are the limits.

Yaml: https://pastebin.com/EXbNyE5y
Discovery Debug: https://pastebin.com/1dz4kcer

@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 16, 2018

You need to pre-cache analogPropertyTable to access the hi-low of analogAlarmLOLO, etc. (see includes/definitions/discovery/routeros.yaml) for an example.

@laf laf added User-Pending and removed Blocker 🚫 labels Sep 16, 2018

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor Author

TheGreatDoc commented Sep 17, 2018

Still stucked, even with pre-cache.

I've tried using:

low_limit: '{{ $analogAlarmLOLO }}'
and
high_limit: '{{ $analogAlarmHIHI.11.1.3.6.1.4.1.17409.1.11.2.0 }}'

With same result. No limit is set in the discovery

@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 17, 2018

You'll need to post the updated files and discovery debug :)

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor Author

TheGreatDoc commented Sep 18, 2018

Sometimes I'm like a newbie.
Disco debug: https://p.libren.ms/view/899379bf
Yaml: https://p.libren.ms/view/e0a73ad5

@laf
Copy link
Member

laf left a comment

Last little bit then this is good to go

@laf
Copy link
Member

laf left a comment

Sorry I think one last thing, the $sensor_index values look too random. You should use for instance commonDeviceInternalTemperature.$index

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor Author

TheGreatDoc commented Sep 21, 2018

I was using the sensor description as sensor index, so the code is the same in all sensors but I dont really care on the name of sensor index.
I've also change the $value inserted in discovery to be divided by $divisor. Before, in discovery, values were inserted without it, making them wrong until poller.

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Sep 22, 2018

CLA assistant check
All committers have signed the CLA.

laf and others added some commits Sep 22, 2018

@laf

This comment has been minimized.

Copy link
Member

laf commented Sep 24, 2018

Hopefully the svg was just the last thing that needed fixing. You can use https://jakearchibald.github.io/svgomg/ to compress the file + you need to swap out width/height for a viewbox value - see existing files.

I've sorted this now so hopefully tests pass.

@TheGreatDoc

This comment has been minimized.

Copy link
Contributor Author

TheGreatDoc commented Sep 24, 2018

Thanks @laf
I was looking how to remove those attributes and found nothing...
Now travis complains about the yaml. It must be kidding us....

@TheGreatDoc TheGreatDoc changed the title WIP New OS: Support for Glass Way EYDFA WDM Optical Amplifier New OS: Support for Glass Way EYDFA WDM Optical Amplifier Oct 9, 2018

@laf

laf approved these changes Oct 12, 2018

Copy link
Member

laf left a comment

LGTM

@laf laf removed the User-Pending label Oct 12, 2018

@laf laf merged commit 1dfa797 into librenms:master Oct 12, 2018

2 of 3 checks passed

codeclimate 2 issues to fix
Details
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@lock lock bot locked as resolved and limited conversation to collaborators Dec 11, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.