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

Added new device - Exagrid #10496

Merged
merged 5 commits into from Aug 7, 2019

Conversation

@nsn-amagruder
Copy link
Contributor

commented Aug 6, 2019

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.

@CLAassistant

This comment has been minimized.

Copy link

commented Aug 6, 2019

CLA assistant check
All committers have signed the CLA.

@PipoCanaja
Copy link
Contributor

left a comment

Hi @nsn-amagruder
Thanx for your Pull Request. Could you please rename the mibfile as suggested in the review ? Will have an overall look at the PR then.
Thanx

mibs/exagrid/* Outdated Show resolved Hide resolved
@PipoCanaja
Copy link
Contributor

left a comment

Mib File rename.

tests/data/exagridos.json Outdated Show resolved Hide resolved
@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

When your changes (if necessary, for version/hardware etc) will be pushed, you can check Travis result and correct whatever issue is found. Right now, the test data is not matching the snmprec, which makes Travis fail.

@nsn-amagruder

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Travis CI showing disk don't match. I think this may be the way their disk is reported in SNMP. This is a backup appliance and they partition the disk differently. Thoughts?

snmprec file
1.3.6.1.2.1.25.2.3.1.3.35|4|/dev/shm
1.3.6.1.2.1.25.2.3.1.3.39|4|/home1

json doesn't report disk 35.
"storage_index": "31",
"storage_type": "hrStorageFixedDisk",
"storage_descr": "/",

                "storage_index": "39",
                "storage_type": "hrStorageFixedDisk",
                "storage_descr": "/home1",

OS exagridos: Discovered storage data does not match that found in tests/data/exagridos.json
Failed asserting that two arrays are equal.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Concerning the tests, I can see 2 explanations :

  • You have some options in your config.php to skip some of the volumes
  • You run the test on a different snmprec than the one attached to this PR.

Anyway, I can run it on my dev machine. We'll see how it goes.

@nsn-amagruder

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Concerning the tests, I can see 2 explanations :

  • You have some options in your config.php to skip some of the volumes
  • You run the test on a different snmprec than the one attached to this PR.

Anyway, I can run it on my dev machine. We'll see how it goes.

I do exclude some volumes in my config.php. Should I add the same excludes to the config.text.php file?

Update exagridos.json
Rerun tests
@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

In fact, you need to do the dev on a "pure" LibreNMS instance. If you have config that impacts discovery or poller (which you most probably have on a productive machine) your tests will reflect this.
And when the automatic tests are run in a "pure" LibreNMS instance by travis, they fail the equality test.

I did run the tests again in a dev machine, that should be better.

@nsn-amagruder

This comment has been minimized.

Copy link
Contributor Author

commented Aug 7, 2019

Appreciate your help. First try for me. Will keep this in mind for the future.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

We all had a first time :) If you can, it is definitely easier to have a dedicated dev box (VM) for playing with GIT and tests.
You can play with different branches without impacting your productive monitoring, and keep your productive instance in sync with auto-updates.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

@nsn-amagruder There we are. Checks OK, ready for merge.

@PipoCanaja
Copy link
Contributor

left a comment

LGTM

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

You'll need to clean your changes in the LibreNMS directory before running daily.php. If not, the auto-upgrade will complain.

@PipoCanaja PipoCanaja merged commit b8c96ab into librenms:master Aug 7, 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

@nsn-amagruder nsn-amagruder deleted the nsn-amagruder:newdevice-exagrid branch Aug 7, 2019

@murrant

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.