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

feature: Add more Dell iDrac state sensors #8254

Merged
merged 7 commits into from Mar 16, 2018

Conversation

Projects
None yet
5 participants
@Rosiak
Contributor

Rosiak commented Feb 14, 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

@laf

This comment has been minimized.

Member

laf commented Feb 18, 2018

Any chance of test data :)?

@Rosiak

This comment has been minimized.

Contributor

Rosiak commented Feb 22, 2018

Will add when I'm back from vacation :)

@murrant

This comment has been minimized.

Member

murrant commented Feb 23, 2018

@Rosiak you need to add the json file tests/data/drac.json too :) If you haven't generated that yet, run ./scripts/save-test-data.php -o drac -m sensors

@f0o

f0o approved these changes Feb 23, 2018 edited

LGTM - preferably add the tests/data/drac.json :)

@laf

This comment has been minimized.

Member

laf commented Feb 27, 2018

I've pushed the json file so this can be merged once it's passed tests.

@laf

laf approved these changes Feb 27, 2018

@laf

This comment has been minimized.

Member

laf commented Feb 27, 2018

Or not. I love our tests but sometimes, I hate our tests :(

@murrant

This comment has been minimized.

Member

murrant commented Mar 12, 2018

I think we need better feedback when things fail, but I have no idea how to do that as sometimes it is something esoteric. If we find the root cause, maybe try to add some output / a test for it. A lot of times the test fails because the actual code is a little wonky.

@murrant

This comment has been minimized.

Member

murrant commented Mar 16, 2018

I get this output every time I run discovery. The reason you couldn't find the test bug is because it is a code bug:
image

I think I want to add an easy way to add the device for testing.
Right now you can do this:

./scripts/save-test-data.php --snmpsim
./addhost.php 127.1.6.1 <snmprec file name> v2c 1161

But that isn't obvious or handy.

@murrant

This comment has been minimized.

Member

murrant commented Mar 16, 2018

Oh and looking at it more closely, the bug is in the old drac state code.........

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Mar 16, 2018

The inspection completed: No new issues

@laf laf merged commit 8fdedcf into librenms:master Mar 16, 2018

2 checks passed

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

This comment has been minimized.

Member

laf commented Mar 16, 2018

Thanks for taking a look at this @murrant

@murrant

This comment has been minimized.

Member

murrant commented Mar 16, 2018

No problem. The fix was -e ;-)

inetAnt added a commit to criteo-forks/librenms that referenced this pull request Mar 19, 2018

device: Added more Dell iDrac state sensors (librenms#8254)
* feature: Add more Dell iDrac state sensors

* add test data

* Added drac.json test data

* Re-run test data

* Fix drac.inc.php
@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

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